Closed Bug 1588142 Opened 4 years ago Closed 4 years ago

about:preferences - migrate the root xul:window element to an html:html element

Categories

(Firefox :: Settings UI, task, P2)

Product:
Firefox
Component:
Settings UI
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: bgrins, Assigned: bytesized)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1588142 - about:preferences - migrate the root xul:window element to an html:html element
47 bytes, text/x-phabricator-request
Details | Review
Bug 1588142 - Move Fluent about:preferences title attribute to a title tag
47 bytes, text/x-phabricator-request
Details | Review
Bug 1588142 - Test fixes
47 bytes, text/x-phabricator-request
Details | Review
Bug 1588142 - Prevent internal pages from flashing the URL on reload r=Gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1588142 - Fix test failure of browser_canvasframe_helper_02.js r=bgrins
47 bytes, text/x-phabricator-request
Details | Review

Similar to Bug 1492582, we should change the DOM in preferences.xul from:

<window>
  <linkset>
    <html:link> ...
    <script> ...
  </linkset>
  CONTENT
</window>

to:

<html>
  <head>
    <link> ...
    <script> ...
  </head>
  <body>
  CONTENT
  </body>
</html>

This will get us the proper box model highlighter in devtools when working on about:preferences, and also be a good test case for surfacing any issues, given the amount of test coverage the preferences page has.

Brendan, are there any intermediate changes from Bug 1492582 that we should split out / land separately to support doing this in preferences.xul? I know you've already landed a few changes in blockers there to help with this.

Flags: needinfo?(bdahl)
Assignee: nobody → ksteuber

Brian, can you please set a priority for this?

Flags: needinfo?(bgrinstead)
Flags: needinfo?(bgrinstead)
Priority: -- → P2

I've come up against a test failure while working on this that I am not sure how to approach: browser/base/content/test/tabs/browser_tab_label_during_reload.js

It appears that this test was added in Bug 1394188 to test a patch that prevents the tab title bar from changing on reload. Unfortunately, the patch I am writing seems to compromise the way this behavior works on the about:preferences page. Previously, the title was defined as an attribute on <window>, whereas now it will be a <title> tag. However, since the title needs to be localized, it is not available on page load, and the empty <title> tag results in the title changing from "Options" to "about:preferences" on reload, then back to "Options" after localization completes.

Dao- I'm not 100% sure what the right approach here is. Is it important that the title not change to "about:preferences" (i.e. could the test be changed to use a page that doesn't require localization)? If the previous behavior is important, do you know the right way to approach this, or who I could ask about it?

Flags: needinfo?(dao+bmo)
Blocks: 1590840

@florian - Would you be able to answer comment 3?

Flags: needinfo?(florian)

As requested, setting needinfo from you, Mossop. See comment 3 for details.

I just pulled from central and did a full rebuild, which resulted in the same test failure as before, so any recent changes to the title mechanism do not appear to fix the issue.

Flags: needinfo?(dtownsend)

Is there a patch here I can look at?

I'm not totally sure how much we care given that on regular load of about:preference we already flash "about:preferences" in the tab anyway.

It's not totally clear to me why we're flipping the title in this case so I'd love to see where that call is coming from. One thought though would be to only insert the title element once we have the translation.

Flags: needinfo?(dtownsend) → needinfo?(ksteuber)

Depends on D51713

given that on regular load of about:preference we already flash "about:preferences" in the tab anyway.

Which I think is a bug. We wanted to not display the URL in case the protocol is about:*.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #10)

given that on regular load of about:preference we already flash "about:preferences" in the tab anyway.

Which I think is a bug. We wanted to not display the URL in case the protocol is about:*.

So we could just implement something to that effect and that would solve the issue here?

Flags: needinfo?(ksteuber)

(In reply to Dave Townsend [:mossop] (he/him) from comment #11)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #10)

given that on regular load of about:preference we already flash "about:preferences" in the tab anyway.

Which I think is a bug. We wanted to not display the URL in case the protocol is about:*.

So we could just implement something to that effect and that would solve the issue here?

Gijs, you mentioned in channel since you worked on bug 1427186 so we were hoping to get some feedback on a path forward here.

The current behavior is that we do flash "about:preferences" in the tab title (I can see this briefly on initial load after typing in about:preferences + Enter, or can easily reproduce by holding ctrl+r with the tab opened). But there's a test in Comment 3 (browser_tab_label_during_reload.js) that thinks we don't do this.

With Kirk's changes (which is switching from <xul:window title=""> to <html><head><title>), that test starts failing. Maybe we are showing the location for longer, or something.

Do you have ideas on how to fix this current behavior so that it doesn't flash the location? I suspect that would also fix it with the patches here applied, although we'd need to confirm it as well. Or alternatively, should we change the test to not worry about this?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
Flags: needinfo?(dao+bmo)

I think there are a few different things going on here.

(In reply to Brian Grinstead [:bgrins] from comment #12)

The current behavior is that we do flash "about:preferences" in the tab title (I can see this briefly on initial load after typing in about:preferences + Enter,

This wasn't meant to be fixed by bug 1394188, which only cared about reloads. The failing test also is supposed to only care about reloads.

or can easily reproduce by holding ctrl+r with the tab opened).

This seems unrelated. It's caused by re-entrancy, basically. DOM fires DOMTitleChanged events asynchronously, and so we manage to process a previous document's event when we've already started loading a different document in the same docshell, which is how we end up in setTabTitle (from https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/browser/base/content/tabbrowser.js#5239-5258 ) without a title. Adding:

        if (event.target != browser.docShell.document) {
          return;
        }

Fixes this. I no longer see about:preferences show up when holding down accel+r. I will put this patch up separately as it seems the right thing anyway.

Note though, that despite the onLocationChange code in the tabbrowser having:

        if (!isReload) {
          gBrowser.setTabTitle(this.mTab);
        }

we still call setTabTitle from there when reloading about:preferences. This is because there's an onLocationChange notification without the reload flag but with LOCATION_CHANGE_SAME_DOCUMENT, because the gotoPref code inside the prefs sets location.hash . We should probably avoid doing that on initial load if the hash is the same as the current hash. I'll file a separate follow-up for that.

In any case, in the current code, when this fires we already have a title.

But there's a test in Comment 3 (browser_tab_label_during_reload.js) that thinks we don't do this.

With Kirk's changes (which is switching from <xul:window title=""> to <html><head><title>), that test starts failing. Maybe we are showing the location for longer, or something.
Do you have ideas on how to fix this current behavior so that it doesn't flash the location? I suspect that would also fix it with the patches here applied, although we'd need to confirm it as well. Or alternatively, should we change the test to not worry about this?

The test waits for the document to load (browserLoaded), at which point the title is expected to be present. Fluent is meant to block DOMContentLoaded for any strings present in the markup, but it doesn't block DOMTitleChanged notifications. DOM code sends one of those immediately when the <title> tag is inserted into the document so it notifies for the empty <title> before fluent has had a chance to do anything about that.

I can see a few solutions:

  1. Mossop's suggestion from comment #6 - only insert the title element once we have translations. This is a bit messy. We'd probably want to have the title in the markup so it blocks DOMContentLoaded, and would then need JS to "transplant" the title into an actual title element... We'd also end up either doing needless re-translation work if we put the data-l10n-id on the actual title, too, or the title wouldn't update on locale change. We can work around both of those, of course, but it would mean we have something like 10 lines of JS/markup just to get a title onto a page without flickering in the parent, and we'd have to do it all again on the next about: page. That doesn't seem appealing to me. Oh, and I'm not sure off-hand if DOM code ever gives up and just sends a "yeah, we have no title" kind of DOMTitleChanged event if there's no title at all.
  2. suppress this event in DOM code for about: pages if the title element has no child nodes and has a data-l10n-id attribute. This is hacky on the DOM level but would work for all our about: pages.
  3. somehow update the frontend code to ignore this event (both in the child and parent process) in the same case. This is a bit trickier because the event fires on the document, not the title element, so finding the title element back and checking it has a data-l10n-id is more work in that code.
  4. ignore all non-about:blank about: URIs in the tab title update code. This didn't use to work well, at least, because there were some pages that indeed do not specify a title at all. Unsure if that's still the case. We could probably fix it if so, but either way it'd create a bit of a footgun where just not specifying a title is no longer an option for an about: page, because you'd end up with a "New Tab" window title, which also looks broken.
  5. generalize our support for the "New Tab" and "Private Browsing" 'empty tab' titles so that the parent process has (cached?) values for about: URI titles itself.

I think the least invasive option with the most correct result would be meshing options 2 and 3: ignore the tab title updates if browser.contentTitle is empty and the update is the result of DOMTitleChanged events. That'd catch these cases and would still mean that onLocationChange will set the URI as a title for pages without any other title set. It'd happen a bit later, but we load about: pages pretty quickly so that's unlikely to matter.

Brian, does that sound OK?

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #10)

given that on regular load of about:preference we already flash "about:preferences" in the tab anyway.

Which I think is a bug. We wanted to not display the URL in case the protocol is about:*.

IIRC there's a separate bug on file for this, and it's not trivial to solve generically per the above, so I think we should punt on that issue here (seeing as we're "just" trying to move prefs from XUL to HTML. For the "regular" load, we'll still flash about:preferences because when we get the initial onLocationChange notification, there's no content title yet - but we have no guarantee that there will be one in the future, so we can't not update the tab title or it'll not update tab titles if you go from another page with a title to about:pagewithouttitle, which would be wrong.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bgrinstead)
See Also: → 1594457
Depends on: 1594472
No longer depends on: 1594472
See Also: → 1594472

Thanks for the detailed analysis, and starting to fix some of this up Gijs!

  1. suppress this event in DOM code for about: pages if the title element has no child nodes and has a data-l10n-id attribute. This is hacky on the DOM level but would work for all our about: pages.

Adding a condition around that notification doesn't sound terrible to me, although I'd want feedback from DOM folks. I wonder if we should do that for all chrome docs instead of just about:pages, since I'm not sure if we'll have an unnecessary title change for the main browser.xhtml window post-Bug 1492582. I'm also not sure if we rely on DOMTitleChange for setting the window title on AppWindows anyway.

That said:

I think the least invasive option with the most correct result would be meshing options 2 and 3: ignore the tab title updates if browser.contentTitle is empty and the update is the result of DOMTitleChanged events. That'd catch these cases and would still mean that onLocationChange will set the URI as a title for pages without any other title set. It'd happen a bit later, but we load about: pages pretty quickly so that's unlikely to matter.

Sounds totally reasonable to me. Would this affect how titles are set on web content with empty <title> tags? If so are there concerns about sites including an empty title tag and never updating it causing a stale title from the previous page?

Flags: needinfo?(bgrinstead) → needinfo?(gijskruitbosch+bugs)

(In reply to Brian Grinstead [:bgrins] from comment #1)

Brendan, are there any intermediate changes from Bug 1492582 that we should split out / land separately to support doing this in preferences.xul? I know you've already landed a few changes in blockers there to help with this.

Clearing this since 1492582 is close to landing.

Flags: needinfo?(bdahl)

(In reply to Brian Grinstead [:bgrins] from comment #14)

I think the least invasive option with the most correct result would be meshing options 2 and 3: ignore the tab title updates if browser.contentTitle is empty and the update is the result of DOMTitleChanged events. That'd catch these cases and would still mean that onLocationChange will set the URI as a title for pages without any other title set. It'd happen a bit later, but we load about: pages pretty quickly so that's unlikely to matter.

Sounds totally reasonable to me. Would this affect how titles are set on web content with empty <title> tags? If so are there concerns about sites including an empty title tag and never updating it causing a stale title from the previous page?

Sorry, yes, I wasn't explicit - I meant doing so for internal (URI scheme about/chrome/resource) pages.

Flags: needinfo?(gijskruitbosch+bugs)

Converting about:preferences's root from a xul:window to an html:html involves moving the Fluent-translated title attribute to a title tag, which starts empty and is populated by Fluent. The initially-empty title tag causes the title bar to flash "about:preferences" on reload.

This patch ignores empty title updates via DOMTitleChanged events to prevent this. This is only done for internal pages (i.e. pages with a system principal).

Depends on D51714

See Also: → 1594752
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87336cc7f8a1
about:preferences - migrate the root xul:window element to an html:html element r=Gijs,bgrins
https://hg.mozilla.org/integration/autoland/rev/1a5f2b044f82
Move Fluent about:preferences title attribute to a title tag r=fluent-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/939b6b3922c2
Test fixes r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2284a535c8bc
Prevent internal pages from flashing the URL on reload r=Gijs

Backed out for failures on browser_canvasframe_helper_02.js

backout: https://hg.mozilla.org/integration/autoland/rev/b42b424565738fdf7dd1edaddf73bda03df040f5

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=devtools&revision=2284a535c8bc5a34c448665c99bdf0d7ea2d73f2&group_state=expanded&selectedJob=276081339

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276081339&repo=autoland&lineNumber=5110

[task 2019-11-13T22:10:54.771Z] 22:10:54 INFO - TEST-START | devtools/server/tests/browser/browser_canvasframe_helper_02.js
[task 2019-11-13T22:10:56.416Z] 22:10:56 INFO - TEST-INFO | started process screentopng
[task 2019-11-13T22:10:56.699Z] 22:10:56 INFO - TEST-INFO | screentopng: exit 0
[task 2019-11-13T22:10:56.700Z] 22:10:56 INFO - Buffered messages logged at 22:10:54
[task 2019-11-13T22:10:56.701Z] 22:10:56 INFO - Entering test bound
[task 2019-11-13T22:10:56.701Z] 22:10:56 INFO - Adding a new tab with URL: about:preferences
[task 2019-11-13T22:10:56.701Z] 22:10:56 INFO - Buffered messages logged at 22:10:56
[task 2019-11-13T22:10:56.703Z] 22:10:56 INFO - Tab added a URL about:preferences loaded
[task 2019-11-13T22:10:56.703Z] 22:10:56 INFO - Building the helper
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - Buffered messages finished
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_02.js | The AnonymousContent was not inserted in the window - false == true -
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - Stack trace:
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - resource://testing-common/content-task.js line 110 > eval:null:31
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - resource://testing-common/content-task.js:null:111
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_02.js | No text content is returned - false == true -
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - Stack trace:
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - resource://testing-common/content-task.js line 110 > eval:null:32
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - resource://testing-common/content-task.js:null:111
[task 2019-11-13T22:10:56.704Z] 22:10:56 INFO - Leaving test bound

Flags: needinfo?(ksteuber)

Heh, looks like this test needs to pick a new document that uses <xul:window> to test that canvas frame doesn't work: https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/devtools/server/tests/browser/browser_canvasframe_helper_02.js#6-11. This is a good thing - it means the normal DevTools highlighter will work in about:preferences now. Maybe you could use data:application/vnd.mozilla.xul+xml;charset=utf-8,<window/> instead of an existing chrome doc?

Flags: needinfo?(ksteuber)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f37c31422e2
about:preferences - migrate the root xul:window element to an html:html element r=Gijs,bgrins
https://hg.mozilla.org/integration/autoland/rev/27b1ed709809
Move Fluent about:preferences title attribute to a title tag r=fluent-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/a6d78953abb6
Test fixes r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2b7c28c1173d
Prevent internal pages from flashing the URL on reload r=Gijs
https://hg.mozilla.org/integration/autoland/rev/48829132cb50
Fix test failure of browser_canvasframe_helper_02.js r=bgrins
Regressions: 1599258
Regressions: 1601708
Regressions: 1606130
Regressions: 1604782
Regressions: 1608246
Regressions: 1611710
You need to log in before you can comment on or make changes to this bug.