Closed Bug 1590180 Opened 7 months ago Closed 4 months ago

Remove usage of `display: -moz-groupbox` and `-moz-appearance: groupbox`

Categories

(Toolkit :: Printing, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: ntim, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Only one usage in mozilla-central: https://searchfox.org/mozilla-central/search?q=display%3A+-moz-groupbox&case=false&regexp=false&path=

Probably not worth keeping the whole nsGroupBoxFrame implementation around just for it.

What does this layout do? Wondering if it could be replaced with a box with some styling or if there's something more involved.

It appears to be some kind of fieldset-like layout for XUL:
https://developer.mozilla.org/en-US/docs/Archive/Mozilla/XUL/groupbox
Maybe the markup can use HTML <fieldset>/<legend> instead? (assuming it's a design we want to keep)

Looking at the source, I don't see anything special other than the painting of the border:
https://searchfox.org/mozilla-central/source/layout/xul/nsGroupBoxFrame.cpp
It would be great if we could get rid of this code.

(In reply to Mats Palmgren (:mats) from comment #3)

Looking at the source, I don't see anything special other than the painting of the border:
https://searchfox.org/mozilla-central/source/layout/xul/nsGroupBoxFrame.cpp
It would be great if we could get rid of this code.

It seems pretty doable to update the windows printPageSetup page to not use it. What do you think we should do with the tests at https://searchfox.org/mozilla-central/search?q=display%3A+-moz-groupbox&case=false&regexp=false&path=?

Flags: needinfo?(mats)

The crashtests might have some value beyond -moz-groupbox, so replacing -moz-groupbox with -moz-box in those seems reasonable. The -moz-groupbox part of the mochitest should be removed.

Flags: needinfo?(mats)

Note that there's also a StyleAppearance::Groupbox (-moz-appearance:groupbox) and some a11y stuff related to this display value:
https://searchfox.org/mozilla-central/search?q=groupbox&path=
I'm guessing those can be removed too.

It appears there are some <groupbox> elements in Preferences etc though, which I assume creates a XULGroupboxAccessible, so let's keep the a11y stuff as is for now. (we can address that after it's been replaced with proper HTML some time in the future)

The -moz-appearance value should be possible to remove now though as printPageSetup.css appears to be the only user:
https://searchfox.org/mozilla-central/search?q=appearance%3A+groupbox&path=
(this might be better to do as a separate bug though, in the interest of keeping the patch size smaller and more targeted)

Assignee: nobody → dtownsend

The display property appears to do nothing here and the appearance closely
matches that of a fieldset so switching to that seems to work fine.

Attached image Screenshot

This is what it looks like with the change.

Whiteboard: keep-open
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/018aa28cf32f
Remove use of `display: -moz-groupbox` and `-moz-appearance: groupbox` from the page setup dialog. r=dao
Keywords: leave-open
Whiteboard: keep-open

Splitting the platform removal off to a new bug, because different patches in different releases on the same bug can get confusing.

No longer blocks: kill-xul-layouts
Status: NEW → RESOLVED
Closed: 4 months ago
Component: Layout → Printing
Keywords: leave-open
Product: Core → Toolkit
Resolution: --- → FIXED
Summary: Remove nsGroupBoxFrame (display: -moz-groupbox) → Remove usage of `display: -moz-groupbox` and `-moz-appearance: groupbox`
Target Milestone: --- → mozilla72
Blocks: 1610404
Blocks: 1612755
You need to log in before you can comment on or make changes to this bug.