1941406 - Let 'layout.details.force-block-layout:false' and 'layout.css.details-content.enabled:true' ride the trains to release

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Daniel Holbert [:dholbert]]

We landed some changes to allow details to have arbitrary display values in bug 1856374, but it's still off by default on all channels (behind the about:config pref layout.details.force-block-layout)

Not sure what more we need to do, but let's use this bug to track enabling the pref and shipping it to release. This may end up being relevant to the interop-2025 project, too (via https://github.com/web-platform-tests/interop/issues/871 ).

Comment 1

[Daniel Holbert [:dholbert]]

This might want to wait for bug 1901037 to be fixed before we flip the pref, but I'm not sure it's a strict blocker. --> adding that as see-also for now.

emilio, do you recall if there was anything else that might be blocking us from just shipping this behavior? Looking back at bug 1856374, it looks like maybe the only reasons we didn't just directly ship it was because nobody else had shipped it yet (so there was potential compat/interop risk) and there was a spec-change needed. But it looks like the spec change happened and Chrome has shipped this (chrome stable 131 passes http://wpt.live/html/rendering/the-details-element/details-display-type-001.html at least) so we're probably good on those fronts now.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Yeah that's right, it was just out of "no spec, no other impls" caution.

Comment 3

[Daniel Holbert [:dholbert]]

great! I'll post a patch with a pref-flip then. FWIW I've had this pref toggled to false in my main browsing profile for ages and haven't noticed any trouble.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941406: Toggle the pref to let HTML <details> element be given custom 'display' values. r?emilio

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/543f1771493e
Toggle the pref to let HTML <details> element be given custom 'display' values. r=TYLin

Comment 6

[Cosmin Sabou [:CosminS]]

Backed out changeset 543f1771493e (Bug 1941406) for causing reftest failures.

Push with failures

Failure log

Backout link

Comment 7

[Daniel Holbert [:dholbert]]

Whoops, sorry about that -- should've tested with reftests.

Looks like just one test failure there, layout/reftests/details-summary/details-display-inline.html, which does indeed set display on a details element. Chrome matches our updated rendering (with the pref flip) so I think the behavior-change is likely-fine.

I'll clean that up and reland within the next day or so.

Comment 8

[Daniel Holbert [:dholbert]]

That test-failure prompted me to modernize/broaden that test, and in doing so, I discovered that the lack-of-::details-content (that's bug 1901037) might conceivably be a rough edge that leads to compat pain, or at least to a weird intermediate state that might be unwanted.

e.g. here's a data-URI testcase to demonstrate this:

data:text/html,<details style="border: 1px solid black; display:flex; flex-direction: column; height: 200px; justify-content: space-between" open><summary style="display:inline">Summary</summary><div>Details1</div><div>Details2</div>

• In current Firefox, it renders with the two "Details" lines adjacent to each other (because we ignore the flex styling)
• In current chrome, it renders with the two "Details" lines adjacent to each other (because it puts those lines in a ::details-content wrapper-box that's display:block by default and forms a single flex item)
• In Firefox-with-the-pref-flipped, we end up splitting the two "Details" lines up (because they each become their own flex item which get spread via space-between).

So: given that we're not in any particular rush to ship this, let's toggle it on for Nightly-only for now (which I'll do in a helper bug) to start to get some testing; and once we have ::details-content support, we can let it ride the trains without worrying about creating a brand-new-not-like-any-other-browser behavior for cases like that^ data URI.

Comment 9

[Phabricator Automation]

Comment on attachment 9459784 [details]
Bug 1941406: Toggle the pref to let HTML <details> element be given custom 'display' values. r?emilio

Revision D234430 was moved to bug 1942190. Setting attachment 9459784 [details] to obsolete.

Comment 10

[Daniel Holbert [:dholbert]]

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

This might want to wait for bug 1901037 to be fixed before we flip the pref

I decided to go with that plan (for non-nightly channels) per comment 8. --> Adjusting bug relationships to reflect that.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941406: Remove the Nightly-only guard from about:config pref "layout.details.force-block-layout". r=emilio

This patch makes this pref unconditionally false, which enables the modern
behavior of letting HTML <details> elements be given any display value.

(Previously, this pref only had this false value on the Nightly channel,
whereas other release channels had true, which gave the old behavior of
forcing <details> to be a block.)

This patch also removes the now-unnecessary .ini annotation to explicitly
set the pref to be false for WPT tests in our own CI.

Comment 12

[Daniel Holbert [:dholbert]]

Posting the (trivial) pref-flip-release patch, so that we've got it ready - though as noted above in comment 8, this will want to wait until we've landed bug 1901037 at least.

Comment 13

[Daniel Holbert [:dholbert]]

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

this will want to wait until we've landed bug 1901037 at least.

(Probably this should wait until that bug's followup-bugs are addressed, too [the ones emilio requested in review in https://phabricator.services.mozilla.com/D241100#8340863 ] --> adding them as dependencies.)

Comment 14

[Daniel Holbert [:dholbert]]

(Really, I guess this is wants to wait until we're ready to toggle layout.css.details-content.enabled to true (that pref is being added in bug 1901037's patch, guarding that patch's code for now). Maybe/probably we can just use this bug here for that pref-flip, too, since it probably wouldn't make sense to ship one of these prefs without the other one.)

Comment 15

[Daniel Holbert [:dholbert]]

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

(Really, I guess this is wants to wait until we're ready to toggle layout.css.details-content.enabled to true[...] Maybe/probably we can just use this bug here for that pref-flip, too, since it probably wouldn't make sense to ship one of these prefs without the other one.

I'll update the patch to do this^.

After that, I'll tentatively mark the patch as layering-on bug 1954142's revision -- that's the only open dependency that remains here, so probably we can land this nightly-guard-removal at the same time as fixing bug 1954142, as an atomic patch-stack. (unless any other blockers come up)
[edit: fixed bug number copypaste typo]

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941406: Enable support for the ::details-content pseudo-element. r=emilio

Comment 17

[Daniel Holbert [:dholbert]]

bug 1954142 has landed and appears to have stuck, so I triggered lando here. (autoland is temporarily closed right now, but this'll merge once it reopens.)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ab837d960197
https://hg.mozilla.org/integration/autoland/rev/b5fc116e847a
Remove the Nightly-only guard from about:config pref "layout.details.force-block-layout". r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/57eaaec9e408
https://hg.mozilla.org/integration/autoland/rev/89504c9ad0c8
Enable support for the ::details-content pseudo-element. r=emilio

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/55dc374b6a99
https://hg.mozilla.org/integration/autoland/rev/a644277cf212
Revert "Bug 1941406: Enable support for the ::details-content pseudo-element. r=emilio" for causing reftests failures in details-percentage-height-children-ref.html.

Comment 20

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mass failures.

• Revert link

---

• Push with failures - mochitests failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/firefoxview/tests/browser/browser_firefoxview.js | Focus should be on the link within the recently closed empty state -

---

• Push with failures - another mochitests failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['slot node', address: [object HTMLSlotElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'paragraph'

---

• Push with failures - reftests failures
• Failure Log
• Failure line: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/details-summary/details-percentage-height-children.html == layout/reftests/details-summary/details-percentage-height-children-ref.html | image comparison, max difference: 255, number of differing pixels: 39750

Comment 21

[Daniel Holbert [:dholbert]]

Whoops, sorry about that - I think I try-tested this previously, but I guess not recently enough. I'll take a look at the failures on Monday.

Comment 22

[Daniel Holbert [:dholbert]]

(Note that the first patch here is just removing a Nightly-only guard, so it should have no behavioral impact. So any behavior-changes are presumably from the second patch, enabling support for ::details-content.)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Serban Stanca [:SerbanS] from comment #20)

• Failure line: TEST-UNEXPECTED-FAIL | browser/components/firefoxview/tests/browser/browser_firefoxview.js | Focus should be on the link within the recently closed empty state -

Fixed by bug 1981724 (once it lands) -- thanks Keith!

• Failure line: TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['slot node', address: [object HTMLSlotElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'paragraph'

Fixed by bug 1981631.

• Failure line: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/details-summary/details-percentage-height-children.html == layout/reftests/details-summary/details-percentage-height-children-ref.html | image comparison, max difference: 255, number of differing pixels: 39750

Fixed by bug 1981803 (once it lands).

I'll do a try run to see if there's any other fallout, and then re-land (once the above-mentioned bugs' patches have landed).

Comment 24

[Daniel Holbert [:dholbert]]

Try run with those bugs' patches included: https://treeherder.mozilla.org/jobs?repo=try&revision=cf4f7932f35c75aada0b21077fc955aac0d6d7e1

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0677021c6f1b
https://hg.mozilla.org/integration/autoland/rev/82d8c7d47593
Remove the Nightly-only guard from about:config pref "layout.details.force-block-layout". r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/760bd6c4ba62
https://hg.mozilla.org/integration/autoland/rev/ebe722998068
Enable support for the ::details-content pseudo-element. r=emilio

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1c72ee85fcc9
https://hg.mozilla.org/integration/autoland/rev/d7cce81c21f4
Revert "Bug 1941406: Enable support for the ::details-content pseudo-element. r=emilio" for causing reftest failures on details-percentage-height-children.html

Comment 27

[Cristina Horotan [:chorotan]]

Backed out for causing reftest failures on details-percentage-height-children.html

Backout link

Push with failures

Failure log

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Cristina Horotan [:chorotan] from comment #27)

Backed out for causing reftest failures on details-percentage-height-children.html

Ah, sorry about that -- huh, so that's the test I fixed in bug 1981803. Aha, I hadn't noticed that Keith folded a different workaround for that same test-failure into the patch on this bug here:
https://phabricator.services.mozilla.com/D257081?vs=1100047&id=1103614#toc

Keith's workaround and my fix in bug 1981803 don't play nicely together, which is why it fails. I guess I didn't catch that in comment 24 because that Try run had the dependencies' patches but not this bug's patches; whoops.

We can remove Keith's workaround from the patch here now, and then I'll do a Try run with that updated patches, and then we can land assuming that Try run looks good.

Comment 29

[Daniel Holbert [:dholbert]]

Hopefully we're all set now.

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=9e225dc78a36e3092bec81b2bdd046d7375fdfef

I'll trigger lando later on assuming that looks good.

Comment 30

[Daniel Holbert [:dholbert]]

Hmm, looks like we've got a few more focus-related issues to sort out:

• mochitest-chrome failure
  "browser/components/firefoxview/tests/chrome/test_card_container.html | Focus should be on the 'View all' link within card-container - got null, expected about:firefoxview#history"
  https://treeherder.mozilla.org/logviewer?job_id=521676578&repo=try
  https://treeherder.mozilla.org/logviewer?job_id=521677912&repo=try

• mochitest-browser-chrome failure:
  "browser/components/firefoxview/tests/browser/browser_firefoxview.js | Focus should be on the link within the recently closed empty state"
  https://treeherder.mozilla.org/logviewer?job_id=521679210&repo=try
  (This was the first failure from comment 20; per comment 23, I tested bug 1981724's patch locally and thought it addressed this, but maybe my testing was incomplete, or maybe there's just some more to this...)

Comment 31

[Daniel Holbert [:dholbert]]

So far I haven't been able to repro the failures from comment 30 locally (with current mozilla-central, running the mochitest with --setpref "layout.css.details-content.enabled=true").

One more failure in my comment 29 try run that we need to address before flipping the pref, though -- and this one I can repro locally:

• mochitest-browser-a11y ("Ba") failure:
  TEST-UNEXPECTED-FAIL | accessible/tests/browser/hittest/browser_test_general.js | Wrong direct child accessible at the point (201, 228) of [DOM node id: detailsOpen, role: details, address: [xpconnect wrapped nsIAccessible]], sought [DOM node id: detailsOpenP, role: paragraph, address: [xpconnect wrapped nsIAccessible]] - Got [xpconnect wrapped nsIAccessible], expected [xpconnect wrapped nsIAccessible]
  https://treeherder.mozilla.org/logviewer?job_id=521676502&repo=try&lineNumber=5851

Comment 32

[Daniel Holbert [:dholbert]]

Hmm, tentative good news... another try run from today is mostly complete and so far I'm not seeing any Firefox View test failures (comment 30) - just the Ba ones (which repro locally and need addressing), and unrelated intermittents:
https://treeherder.mozilla.org/jobs?repo=try&revision=7969085edc2c8f5e4ed145f86fcb6fceeb7df0a4

Comment 33

[Daniel Holbert [:dholbert]]

Yeah, the Firefox View test-failures seem to be gone. No idea offhand how/why those were still showing up in comment 30, but it seems something has changed since then that addresses them. Hooray, I guess!

I filed bug 1982410 on the Ba failures which are the only "real" failures I'm seeing in comment 32, and I've got a band-aid patch up for review there.

Here's a try run with that band-aid plus the patches on this bug here (selecting nearly all reftests/mochitests/crashtests/wpt, nearly all platforms, hoping to catch any possible issues we may have somehow still overlooked):
https://treeherder.mozilla.org/jobs?repo=try&revision=3a25a4474b3d31af45afef79516ac238c526083e

Comment 34

[Daniel Holbert [:dholbert]]

Try run from comment 33 looks good to me (all unrelated intermittents). I just triggered lando for bug 1982410 and I'll trigger lando here again later this evening.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ade8b3ac34b0
https://hg.mozilla.org/integration/autoland/rev/4dde55dd0017
Remove the Nightly-only guard from about:config pref "layout.details.force-block-layout". r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/b6ac88edded7
https://hg.mozilla.org/integration/autoland/rev/2e1d3930236b
Enable support for the ::details-content pseudo-element. r=emilio

Comment 36

[Sebastian Zartner [:sebo]]

As this bug seems to be meant to let arbitrary display values for <details> elements and ::details-content ride the trains, I added the dev-doc-needed flag and I am requesting a release note.

[Why is this notable]: Allows authors to style the contents of a <details> element.
[Affects Firefox for Android]: yes
[Suggested wording]: Restrictions were removed that prevented setting the display property on <details> elements, and a ::details-content pseudo-element was added to style the expandable/collapsible contents of those elements.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/::details-content

Sebastian

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4dde55dd0017
https://hg.mozilla.org/mozilla-central/rev/2e1d3930236b

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Added to the Fx143 relnotes, thanks.

Comment 39

[Brian Thomas Smith [bsmth]]

MDN changes for this can be tracked in the following GitHub issue: https://github.com/mdn/content/issues/40777

Comment 40

[Daniel Holbert [:dholbert]]

I sent an intent-to-ship last week, BTW (and patches are already landed to let this ride the trains, in comment 37):
https://groups.google.com/a/mozilla.org/g/dev-platform/c/O_pX2nEyCFU