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
This patch fixes a number of issues that clear up a few different
intermittents in browser_tab_preview.js.
1. In browser_tab_groups_tab_interactions_telemetry.js, a call to "close
duplicate tabs" would automatically open the confirmation hint panel.
If not manually closed, it seems to persist in a half-way open state
where the `animating` attribute is always `true`. This is now fixed.
2. There is a test somewhere that seems to apply the
`sidebar.verticalTabs` pref and doesn't turn it off. I would like to
figure out which test is causing this, but as a last defence I have
made sure this is always properly set within this test suite.
3. Some tests check that THP is disabled if another panel is open. These
used to work by opening the `appMenu-popup` menu (the hamburger menu
in the top right of the browser). The problem with this approach is
that this panel (and basically every other panel) are very complex
and are prone to change. This was causing an intermittent failure
where attempting to close this panel threw an error in console that
seemed to be specifically related to this panel. I fixed this by
creating a fake minimal panel that is guaranteed to not change as
application code changes. (I borrowed this technique from
https://searchfox.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser_popupNotification_queue.js).
4. There is a suite of tests that checks that THP is disabled in the
event that a drag operation is performed. The problem is that
dragging a tab by 100px sometimes detaches the tab and creates a new
window, breaking the test run. Reducing the drag amount seems to fix
this.
5. As a last line of defence, I have added a state reset function at the
start of each test to ensure that all panels are closed and that the
mouse is not hovering over the tab strip. (The latter was being done at
the end of some tests already, but this makes sure it is consistently
applied everywhere.)
Closes bug1898099, bug1905944, bug1933597.
Differential Revision: https://phabricator.services.mozilla.com/D258976
I looked into using gInitialPages instead of hard-coding "about:blank",
but:
- there's no convenient window or browser here to access gInitialPages
- We hard-code "about:blank" many other places in source, so its
"specialness" is already pretty well established.
Original Revision: https://phabricator.services.mozilla.com/D254871
Differential Revision: https://phabricator.services.mozilla.com/D258221
One annoying part to this is that for "Save As" downloads on a PDF (for
example), we don't get a valid browsing context. For downloads, the only
thing we need a browsing context for us to get a window so we can show
the "DLP busy" notification, so just use the window that we were
initialized with. In practice I can't tell any difference.
Original Revision: https://phabricator.services.mozilla.com/D251880
Differential Revision: https://phabricator.services.mozilla.com/D258204
Unable to reproduce the original issue, but adding a safeguard
to ensure search terms only persist when the current URI matches
the origin of the URI used to load the page.
Differential Revision: https://phabricator.services.mozilla.com/D253982
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
- add telemetry "tab" field for start, cardlink, cardClose
- add labsCheckbox event for shift, shift_option, long_press, key_points, link_previews
- add _getTabContextValue to get the filepath for any about: pages
- update optin telemetry
- add metrics
- shift_alt and long_press name normalization
- add key_points (boolean) to card_link
- update test to test against blank
- refactor _getTabContextValue
- add tests for new metric and prefChanged
Original Revision: https://phabricator.services.mozilla.com/D251779
Differential Revision: https://phabricator.services.mozilla.com/D252692
Please see the bug for context. This patch is intended for uplifting directly to
140/beta without landing on 141/m-c. It depends on bug 1965903 D252024 being
uplifted first.
Depends on D252024
Differential Revision: https://phabricator.services.mozilla.com/D252026
This makes a couple other changes that aren't strictly necessary:
In `makeResult()`, instead of reusing the `suggestion` variable for the Merino
suggestion, use a new `merinoSuggestion` variable. It's less confusing (and
should be better for eventual TypeScript integration?).
In `QuickSuggestTestUtils`, slightly reorganize the geonames RS data so the
order of the properties is consistent with the similar mock data in the Rust
component and geonames uploader Merino job. Also use real geoname IDs.
Differential Revision: https://phabricator.services.mozilla.com/D252024
