I wasn't around when RFPTarget::SiteSpecificZoom was implemented, but the current implementation is very confusing.
In Firefox, we have two modes of zoom level. Per tab zoom, and site specific zoom. It is controlled in two (maybe more) places. browser-fullZoom.js and in [CanonicalBrowsingContext.cpp](https://searchfox.org/mozilla-central/rev/ac81a39dfe0663eb40a34c7c36d89e34be29cb20/docshell/base/CanonicalBrowsingContext.cpp#285-289).
Our current implementation disables site specific zoom in `browser-fullZoom.js` by checking the RFP target, but it doesn't modify `CanonicalBrowsingContext.cpp`. So,`CanonicalBrowsingContext` still thinks we are using site specific zoom.
Pre-bug1914149 this method worked because we didn't keep/inherit zoom level across navigations. Post-bug1914149, it no longer works because we keep the zoom level across navigations in `CanonicalBrowsingContext` and let `browser-fullZoom.js` reset to its correct value back.
The issue is caused because `CanonicalBrowsingContext` keeps the previous page's zoom level, but `browser-fullZoom.js` thinks we use tab zoom mode, so it doesn't bother setting the zoom level for the site/page. So, we end up keeping the zoom level.
The solution here I'm suggesting is, doing the opposite of what we are doing in `browser-fullZoom.js`. So, now we should force SiteSpecificZoom with RFP. The reason I'm suggesting this is because within the same site, even acrss tabs, we persist cookies, so fingerprinting isn't much of concern here. We also don't persist zoom levels in private browsing. So, linking normal to PBM isn't a concern either.
So, in summary,
- If you open the same site, in 100 tabs, all of them will get the same zoom level with this patch (just like default normal Firefox)
- If you are in PBM, the zoom level is NOT persisted.
- If you are in normal browsing, the zoom level is persisted, but so are cookies.
Original Revision: https://phabricator.services.mozilla.com/D257497
Differential Revision: https://phabricator.services.mozilla.com/D261948
The required range for the `animDropElementIndex` is between 0 and `this.ariaFocusableItems.length`, inclusive. That maximum value signifies that all tab strip items (and particularly the last one) need to shift out of the way because the user is dragging something to the end of the tab strip.
When dragging past the last tab strip item, `animDropElementIndex == this.ariaFocusableItems.length` and there is no overlapping element. In that scenario, we fall back to setting `dropElement = this.ariaFocusableItems[oldDropElementIndex]` but there is no such element. The `dropElement` is undefined, which ultimately results in calling `Tabbrowser.moveTabsAfter` and appending the dragged tab(s). That happens to be the right result.
However, if you do a short vertical drag, `dropElement` also does not get set, so whatever you were dragging moves to the end of the tab strip. That was bug 1959350 and we tried to fix it by not moving tabs if `dropElement` wasn't set. https://phabricator.services.mozilla.com/D247339
This had the side effect of not making it possible to drop a tab at the end of the tab strip if you were dragging forward. It was still possible to drag forward to the last position, then drag backward slightly to get the front edge of the tab to overlap the last tab in the tab strip, which would allow dropping into the last position.
I think the main thing that would help is to make sure that `dropElement` is always set for valid moves. When dragging past the last tab and there is no overlap, we can fall back to the last non-moving tab strip item.
I needed to swap around how we derive `dropElement` and `newDropElementIndex`. If we fall back to making `dropElement` the last tab, the existing code will try to use `dropElement.elementIndex` as the `newDropElementIndex`, which means the `animDropElementIndex` can never achieve the maximum value that it needs to show all of the tabs moving over. I made it so that we independently determine the `newDropElementIndex` fallback and the `dropElement` fallback without using `newDropElementIndex`.
After this change, `dropElement` should always be a valid non-moving tab strip item if a move should occur. If `dropElement` is undefined, then it's correct that no move should occur.
Differential Revision: https://phabricator.services.mozilla.com/D249715
Fixes a regression that disables tab group management immediately after creating a group with STG off.
I don't quite understand why this comes up, so I hope @ngrato can shed some insight before we land it.
Differential Revision: https://phabricator.services.mozilla.com/D250344
To ensure that a group ends up at the position specified by `index`,
this patch adjusts the index when the tab group moves to the right.
Before this patch, the tab group appeared at a too low (left) index
because the original logic did not account for tabs shifting after a
repositioning to the right.
This also introduces stricter validation for moving tabs near pinned
tabs or other tab groups. Previously, the tabbrowser internals adjusted
the index as needed to fit adjacent to pinned tabs or groups. Now, the
extension API throws an error.
The new behavior matches developer expectations and Chrome's behavior:
https://bugzilla.mozilla.org/show_bug.cgi?id=1963825#c9
Differential Revision: https://phabricator.services.mozilla.com/D249493
The `replaceGroupWithWindow` method currently adopts a tab group in
multiple steps, asynchronously. This can result in externally observable
inconsistencies past initial adoption (see bug).
To fix this, this patch changes the adoption logic by passing the tab
group as the initial item to adopt, and adopts the whole group at once
as needed. Now the logic is similar to drag and drop adoption as
implemented in bug 1908441.
Since tabToAdopt is no longer just a tab, but also a tab group (or even
a tab group label since bug 1908441), the logic in ext-browser.js needs
to be adjusted to make sure that it does not mistake non-tab elements
for tabs. Test coverage is in browser_ext_tabGroups_move_onMoved.js,
as not changing that caused the test to fail with:
> FAIL Tab did indeed move to the new window - {"oldWindowId":3,"oldPosition":"undefined"} deepEqual {"oldWindowId":3,"oldPosition":1} -
Differential Revision: https://phabricator.services.mozilla.com/D248537
To allow extensions to correctly mirror the positions of tabs that were
bulk-moved by moving a tab group, emit the TabMove event in the reverse
order if the tab group moves forwards. This ensures that the previously
recorded index of a tab before the bulk-move remains valid.
Differential Revision: https://phabricator.services.mozilla.com/D248995
The tabGroups.onRemoved / onCreated / onMoved event depends on the
isAdoptingGroup / removedByAdoption flags to identify tab groups that are
adopted by another window. Although this flag is set for the
`gBrowser.adoptTabGroup` method, it is not in the
`gBrowser.replaceGroupWithWindow` method, which results in incorrect
events.
This patch fixes the issue by setting the isAdoptingGroup flag in
`gBrowser.replaceGroupWithWindow`.
Differential Revision: https://phabricator.services.mozilla.com/D248327
Record when a user ungroups the tabs of a tab group.
We ungroup an entire group when the user selects the "Ungroup tabs" context menu item on the tab group, but we also do that if the user cancels out of tab group creation context menu. We are particularly interested in understanding when users cancel out of the creation process. This may give us a better indicator about accidental tab group creation, e.g. when the user intended to drag and drop to move a tab but ended up creating a tab group from two tabs.
Differential Revision: https://phabricator.services.mozilla.com/D247633
Session restore code doesn't use the standard Tabbrowser.addTabGroup API, so tab groups that enter the DOM via session restore don't raise any TabGroupCreate events. To be consistent, and in order to support addons that need to have an up-to-date view of the tab strip, this patch fires TabGroupCreate on each tab group that enters the DOM.
This patch renames the existing `TabGroupCreate` fired from Tabbrowser.addTabGroup to `TabGroupCreateByUser`. This new event will fire after the tab group enters the DOM (therefore after `TabGroupCreate`) in scenarios where code calls Tabbrowser.addTabGroup with `isUserTriggered = true` to indicate that a user took an explicit action to create this tab group as a new tab group.
Differential Revision: https://phabricator.services.mozilla.com/D246630
This is more visible on Linux because the decorations there are bigger, but I
believe this also fixes issues on windows specially if you have the titlebar
checkbox on.
You need to use mozInnerScreen* coords in order to get the screen coordinates
of the client area.
Differential Revision: https://phabricator.services.mozilla.com/D246829
Tabs can also be unloaded for low memory conditions, so only add the
"discarded" attribute if the tab was unloaded explicitly.
Differential Revision: https://phabricator.services.mozilla.com/D246766
We were initializing two SmartTabGroupingManager instances in tabbrowser and tabgroup-menu. This was
causing issues with telemetry. This is now cleaned up.
Also, adding the model reason for no suggestion per DS request.
Differential Revision: https://phabricator.services.mozilla.com/D246113
Records counts of user interactions with tab groups:
- expand a collapsed tab group
- collapse an expanded tab group
- rename a tab group
- change a tab group's color
- save and close a tab group
- delete a tab group
- reopen a deleted tab group
- ungroup a tab group
- move a tab group to a new window
- reopen a saved and closed tab group from the list all tabs menu
- reopen a saved and closed tab group from a URL bar suggestion
- reopen a recently closed tab group from the recently closed tabs menu
Differential Revision: https://phabricator.services.mozilla.com/D246123
For the first and last tab in a tab group, include the tab group's name in the accessibility description read by screen readers when the tab is in focus. As keyboard users go through the tab strip, the users will receive clues that they are passing into/out of a tab group.
This also reformats the tab tooltips in general.
Before:
```
$title $pid $active - $containerName
$audioPlaying
```
After:
```
$title
$pid $active
$tabGroupName - $containerName
$audioPlaying
```
For each portion of the tab tooltip string, parts of the string might not appear due to configuration (PIDs and active only shown with pref) or user state (container name only shown for a container tab, etc.) or UX choices (don't include the title in the accessibility description because screen readers already read out a tab's title by default)
Differential Revision: https://phabricator.services.mozilla.com/D245186
This patch adds the most basic tab interaction metrics for tab groups.
As discussed in standup, we agreed to move the `remove_` class of
metrics into its own bug due to extra complexity involved in correctly
capturing these events.
One other thing we discussed was what the scope of events should be for
the `close_` class of events, i.e. should we *only* capture an event
when someone clicks the "X" button or closes from the TOM menu, or
should we also account for things like context menus, keyboard
shortcuts, etc.? Originally we agreed to capture _all_ tab close events
and mark any source that was not one of the above mentioned two as
unknown. However, after thinking about this more, I don't believe this
is the right approach. There are many places in the codebase where a tab
is closed but not because a user deliberately did it (e.g. when moving a
tab to a new window — this is actually done by creating a new tab in a
new window and closing the old). We currently don't distinguish between
user-initiated close actions, so it would take some time to find all
these places and exclude them.
I favour an explicit inclusion approach (which is what we are doing
elsewhere). In this patch I added events for:
- closing a tab from the "X" close button (`close_tabstrip`)
- closing a tab from the tab context menu (`close_tabstrip`)
- closing a tab from the TOM (`close_tabmenu`)
There are other places that could potentially be
addressed, so I suggest moving these to a follow-up bug and addressing
them for 139.
Differential Revision: https://phabricator.services.mozilla.com/D244915
