Closed Bug 1582530 Opened 5 years ago Closed 5 years ago

Turn on `layout.css.xul-box-display-values.survive-blockification.enabled` by default

Categories

(Core :: Layout, task)

Product:
Core
Component:
Layout
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(2 files)

Bug 1582530 - Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1582530 - Fix remaining cases that were relying on blockification.
47 bytes, text/x-phabricator-request
Details | Review

It should be pretty much good after bug 1582316 and bug 1581973, but I'll make a try push with the assertion patch to make sure.

Thanks for filing!

(RE "turn it on by default or remove the pref" - I'd advocate for just turning it on, via the .yaml file, so that we've still got a quick and easy way to roll back if needed, and to locally test with/without this behavior if we think we might need to roll back).

I'm in meetings for the next few hours but I'll gladly r+ a patch assuming this doesn't break anything (thanks for doing a try push).

No longer blocks: 1580012
Depends on: 1580012

We discussed and are going to aim for flipping the pref in this bug to give it some time to bake on nightly before the pref gets removed.

Summary: Turn on `layout.css.xul-box-display-values.survive-blockification.enabled` by default or remove the pref → Turn on `layout.css.xul-box-display-values.survive-blockification.enabled` by default

This is not quite ready for review, there are two remaining crashes AFAIK: the mozscreenshots related one and the tab overflow test one.

Attachment #9094335 - Attachment description: Bug 1582530 - Fix remaining cases relying on position: absolute; blockification. → Bug 1582530 - Fix remaining cases that were relying on blockification.
Assignee: nobody → ntim.bugs
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b62377b233a8
Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/deba67add7d2
Fix remaining cases that were relying on blockification. r=dao
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d8d8016da4f
Fix crashtest failures. r=bustage on a CLOSED TREE

This change to 360339-2.xul is problematic and will probably cause e.g. the <style> element itself to show up visibly on the page. (Normally it's display:none.)

<html:style>
-* { float: right; }
+* { float: right; display: block; }

https://hg.mozilla.org/integration/autoland/rev/0d8d8016da4f#l2.13

Please add one more followup, to instead apply that display:block in a more targeted way (to just the elements that matter), like we did for the -1 version of the crashtest here:
https://hg.mozilla.org/integration/autoland/rev/deba67add7d2#l6.2

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7fb765cb6727
Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1ae40ac76cd0
Fix remaining cases that were relying on blockification. r=dao
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7eb69d3d03a5
Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c0fcdc789da3
Fix remaining cases that were relying on blockification. r=dao

(In reply to Cristina Coroiu [:ccoroiu] from comment #16)

Mochitest failure log (2):
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=268702866&repo=autoland&lineNumber=12726

Hi Daniel, Is the fullscreen assertion failure something you could look at ? I couldn't figure out what element is affected in this specific case.

Thanks :)

Flags: needinfo?(ntim.bugs) → needinfo?(dholbert)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc547158dbf5
Backed out 2 changesets for causing reftest permafailures in /builds/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2188 CLOSED TREE

I haven't been able to reproduce that assertion failure during dom/html/test/test_fullscreen-api.html in my local Linux environment, but perhaps that's not surprising because it looks like the affected push only shows that issue happening on Windows and Mac. (There are several linux platforms with all-green M-1 through e.g. M-16` runs.)

I've adjusted the assertion to move it a bit later (after the frame's been added to the frame tree, so that it can be dumped), and I added logging to print the frame's address and dump the frame tree, and I've pushed that to Try (as a child of the exact commit for comment 18's failure):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=622426a82abd49f2fbc11d7602d148d70a84a713

Hopefully it crashes for the same frame and the diagnostic info is useful for tracking it down...

Built locally on Windows and poked around a bit. So that fullscreen-api failure is happening for a XUL element that is the child of an HTML body, and whose tag-name is literally "element".

I'm pretty sure it's this "element" here, since this is the testcase that we're running when the failure happens:

function testNamespaces(followupTestFn) {
  let tests = [
  [omitting some lines here for brevity]
    {allowed: true,  name: "element", ns: "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"},

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/dom/html/test/file_fullscreen-api.html#292

Flags: needinfo?(dholbert) → needinfo?(ntim.bugs)

It looks like the test is validating that a XUL element can be the document.fullscreenElement (among other things).

And fullscreening an element makes the element fixed-pos, via this line:

*|*:fullscreen:not(:root) {
  position: fixed !important;

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/layout/style/res/ua.css#330

So whenever a XUL element becomes fullscreen, we also need to give it "display:block" in order for it to actually behave as a proper fixedpos . :-/

Obvious question: do we even use the fullscreen APIs with XUL, in practice? If we could prove that we don't, that would be helpful to let us e.g. remove this part of the testcase and not worry about it.

(In reply to Daniel Holbert [:dholbert] from comment #22)

It looks like the test is validating that a XUL element can be the document.fullscreenElement (among other things).

And fullscreening an element makes the element fixed-pos, via this line:

*|*:fullscreen:not(:root) {
  position: fixed !important;

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/layout/style/res/ua.css#330

So whenever a XUL element becomes fullscreen, we also need to give it "display:block" in order for it to actually behave as a proper fixedpos . :-/

Thanks for looking into it!

I've added:

xul|*:fullscreen:not(:root) {
  /* The position: fixed; property above used to force the computed display
   * value to block. It is no longer the case now, so we manually set it here to
   * maintain the old behaviour. */
  display: block;
}

but I think that might not fix the cases where we explicitly set display: -moz-box and use fullscreen at the same time (if we ever do), unless that display: block; has !important, which might be problematic in other cases (hidden element for example), though I'm not sure a hidden element can actually get the fullscreen state.

Obvious question: do we even use the fullscreen APIs with XUL, in practice? If we could prove that we don't, that would be helpful to let us e.g. remove this part of the testcase and not worry about it.

Bug 1305928 comment 1 says it's needed but I don't actually know if that's still true. Thomas, do you remember where this was needed ?

Flags: needinfo?(ntim.bugs) → needinfo?(wisniewskit)

ntim: I just remember a test failing without that, not sure if it's still necessary.

Flags: needinfo?(wisniewskit)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae88c31cf531
Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/536e78fea3de
Fix remaining cases that were relying on blockification. r=dao,dholbert
Depends on: 1584638
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ce8534b2d85
Backed out 2 changesets for Creshtest failures in ayout/generic/ReflowInput.cpp. CLOSED TREE
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b768fb9e4983
Turn on 'layout.css.xul-box-display-values.survive-blockification.enabled' by default. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/72a8d8c20180
Fix remaining cases that were relying on blockification. r=dao,dholbert

https://hg.mozilla.org/mozilla-central/rev/b768fb9e4983
https://hg.mozilla.org/mozilla-central/rev/72a8d8c20180

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Regressions: 1584780
Regressions: 1586369
Regressions: 1586884
Regressions: 1589351
Blocks: 1600998
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: