Not sure if this is worth it, your call. But I wrote this so I figured
someone debugging this code might find it useful.
Depends on D172033
Differential Revision: https://phabricator.services.mozilla.com/D172034
Add logic to apply theme colors to Feature Callout based on where it's
going to show. We can use in-content CSS properties for Firefox View and
other themed system pages, but not for PDF.js, nor for any callouts we
might show in the browser chrome in the future. For the browser chrome
in general, we can use the lightweight theme properties directly, in the
same way the chrome frontend does. But PDF.js is a special case, since
although it exists in the chrome, it's meant to appear like it's in the
PDF.js viewer. And the PDF.js viewer has its own theme totally
independent of everything else. So this dynamically applies themes from
different sources.
This also fixes the bug where the PDF.js color scheme could mismatch the
PDF.js viewer if the browser theme and system color scheme don't match,
e.g. where system color scheme is light but a dark theme is installed,
or vice versa. For PDF.js specifically, we can use the
-moz-content-prefers-color-scheme media query to follow the color scheme
as it exists in the PDF.js viewer page instead of the color scheme in
the chrome window where the Feature Callout actually exists.
It also adds or modifies some colors that were previously missing or
different from the prototype, fixes the illegibility of buttons in HCM
and forced colors mode, and makes some other minor color changes.
Differential Revision: https://phabricator.services.mozilla.com/D173088
Add logic to apply theme colors to Feature Callout based on where it's
going to show. We can use in-content CSS properties for Firefox View and
other themed system pages, but not for PDF.js, nor for any callouts we
might show in the browser chrome in the future. For the browser chrome
in general, we can use the lightweight theme properties directly, in the
same way the chrome frontend does. But PDF.js is a special case, since
although it exists in the chrome, it's meant to appear like it's in the
PDF.js viewer. And the PDF.js viewer has its own theme totally
independent of everything else. So this dynamically applies themes from
different sources.
This also fixes the bug where the PDF.js color scheme could mismatch the
PDF.js viewer if the browser theme and system color scheme don't match,
e.g. where system color scheme is light but a dark theme is installed,
or vice versa. For PDF.js specifically, we can use the
-moz-content-prefers-color-scheme media query to follow the color scheme
as it exists in the PDF.js viewer page instead of the color scheme in
the chrome window where the Feature Callout actually exists.
It also adds or modifies some colors that were previously missing or
different from the prototype, fixes the illegibility of buttons in HCM
and forced colors mode, and makes some other minor color changes.
Differential Revision: https://phabricator.services.mozilla.com/D173088
Either of these changes (ie dropping the setTabState call for batch restored
tabs, or ensuring the restoreTabs code correctly fills its array with dummy
entries) is sufficient here. I chose to do both because I think in both cases
the brokenness is not limited to this scenario or the issues at hand.
Specifically, the setTabState call was added in bug 1521346 to deal with
moved lazy tabs, but is now being invoked for session restore because of
the batchInsertingTabs optimization work. It doesn't actually need to be,
as far as I can tell, and the lacking _tPos in this case (because we don't
insert the tab into the tabstrip a few lines above) is what breaks things
inside _ensureNoNullsInTabDataList. Note that this _already_ was breaking
things in restoreTab(), which would assign into tabs[undefined] on the
window state object, so just dropping the call seemed better than wallpapering
the absence of _tPos.
The restoreTabs code, pre-patch, calls _ensureNoNullsInTabDataList but that
will never do anything, because right before calling it we change the array
length, so maxPos was always smaller than the size of the list. This meant
we still had empty slots in the array, which was also causing confusion down
the line.
I added the explicit exception for the broken _tPos in restoreTab so that we
notice any future issues with this more quickly. Doing so without any of the
other fixes broke the pre-existing browser_586068-apptabs.js test, so
hopefully that will catch any future changes that break the code's assumptions.
Differential Revision: https://phabricator.services.mozilla.com/D173070
I'm removing the apptab bits completely in one of the later commits,
but trying to keep this modular so it's easy to figure out regressions,
should there be any.
Differential Revision: https://phabricator.services.mozilla.com/D171411
The unnecessary tabbrowser-tab-tooltip is dropped, as it's the same single variable references in all locales. Also, setting the title as the label directly avoids a minuscule but observable delay that's the source of this issue.
Differential Revision: https://phabricator.services.mozilla.com/D171103
This moves the somewhat out-of-place `_loadURI` method and its dependencies
into tabbrowser, and deals with receiving either a string or a URI.
Depends on D168391
Differential Revision: https://phabricator.services.mozilla.com/D168392
This moves the somewhat out-of-place `_loadURI` method and its dependencies
into tabbrowser, and deals with receiving either a string or a URI.
Depends on D168391
Differential Revision: https://phabricator.services.mozilla.com/D168392
Set up event handlers so that pressing Escape dismisses Feature
Callouts. If an interactive element outside of the Callout is focused,
then the Escape key will not be consumed. Also consolidate all the event
handlers into a single switch statement so we won't need to continue
adding more callback bindings (they were only necessary before
encapsulation was implemented). Also change the names of a couple
formerly pseudo-private methods that we're now referencing externally.
Differential Revision: https://phabricator.services.mozilla.com/D168307
Unify the values of "source" and "page" as used in FeatureCallout.sys.mjs:
- Explicitly pass in a value for "page" when instantiating a Feature Callout and use this for the value of "page" when sending Feature Callout telemetry and as the "source" when making calls to `sendTriggerMessage`. This avoids the risk of including non-about: page URLs or PDF file extensions in our telemetry.
- Set the value of "page" in an HTML data attribute that can be accessed for use in about:welcome telemetry for Spotlight and Feature Callouts.
- Update references to the page value previously used as the page/source for telemetry from `about:firefoxview` Feature Callouts from "firefoxview" to "about:firefoxview"
- Pass the token "chrome" when creating a callout from the browser chrome and update references to the source in PDF.js messages' targeting
- Update the page value expected in automated tests as needed
Differential Revision: https://phabricator.services.mozilla.com/D165910
gBrowser.tabs was either redirecting to gBrowser.tabContainer.allTabs or
using the previously cached result. However, most tabContainer code uses
allTabs directly, which was not benefiting from the cache.
Therefore, this patch caches gBrowser.tabContainer.allTabs, and makes
gBrowser.tabs always redirect to it.
Also makes it consistent for gBrowser.tabContainer._getVisibleTabs() and
gBrowser.visibleTabs. In that case both the logic and the cache were in
gBrowser, and tabContainer was redirecting to that, except when gBrowser
hadn't been initialized.
So it's better to have both the logic and the cache in tabContainer.
Differential Revision: https://phabricator.services.mozilla.com/D166962
And make callers rely on that instead of allowInheritPrincipal when
creating lazy tabs.
Unlike allowInheritPrincipal, skipLoad sets the nodefaultsrc attribute.
This avoids a load instead of falling back to about:blank.
One consequence of that is that switching to a lazy about:blank tab will
not notify invoke listeners registered with addTabsProgressListener
(listeners registered with addProgressListener will still be invoked).
Thus test browser_open_in_lazy_tab.js needs to be updated.
Differential Revision: https://phabricator.services.mozilla.com/D166012
This used to do three things:
1. determine whether we're background loading
2. determine a default owner tab
3. force allowInheritPrincipal to a bool value
For the background loading, I removed the default pref check and moved the
selection action into addTab. I defaulted it to true, so that existing addTab
callers continue to open background tabs. I removed the code checking the pref:
all existing loadOneTab callsites pass an explicit value and where appropriate
check the pref themselves (sometimes doing the opposite in response to shift
keypresses etc.).
The one exception is browser_bug597218.js, but there the default of true gets
used right now, which is still the same post-patch.
For the owner tab, I moved setting the default into addTab. This shouldn't
affect existing addTab callers as they don't pass inBackground anyway, so we
default inBackground to true, in which case it just assigns null, only when
ownerTab wasn't provided in the first place.
For allowInheritPrincipal, the only reads of this in addTab use boolean negation
anyway so forcing its type to bool first made no difference.
Differential Revision: https://phabricator.services.mozilla.com/D165774
