Remove the "caption" and "groupbox" bindings

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
22 days ago

People

(Reporter: surkov, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
Anonymous groupbox-body is only used to propagate xul:box attributes like orient,pack,align, which questionable syntax sugar, because the majority of all groupboxes don't use attributes at all, and what makes grooupbox-body just an extra element in hierarchy.

Also there's some styling applied to anonymous .groupbox-body element outside the groupbox binding (for example in pageInfo.css), which sort of breaks encapsulation rules.

Last but not least, it will help to get rid the groupbox XBL binding.

Also see bug 1429940 comment 12 for details.
(Reporter)

Comment 1

7 months ago
it appears it is artifact and no longer used, kill it
Attachment #9011694 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 2

7 months ago
Comment on attachment 9011694 [details] [diff] [review]
part1: remove unused style for pageInfo

Thanks! I probably removed the last usage in bug 1451282 and missed this.
Attachment #9011694 - Flags: review?(paolo.mozmail) → review+

Comment 3

7 months ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f4bc25eec8
part1, remove unused style for pageInfo, r=paolo
(Reporter)

Updated

7 months ago
Keywords: leave-open
(Reporter)

Comment 4

7 months ago
Attachment #9012068 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 5

7 months ago
Comment on attachment 9012068 [details] [diff] [review]
part2: remove .groupbox-body dependency in pref dialog

Redirecting to Matt since it seems he originally reviewed this code, but it seems to me that the padding on the "browser" element and on the "groupbox-body" anonymous element are two different things. Maybe we don't need some of the code anymore? Matt can probably suggest a way forward.
Attachment #9012068 - Flags: review?(paolo.mozmail) → review?(MattN+bmo)

Updated

7 months ago
Priority: -- → P3
Blocks: 1429940
Comment on attachment 9012068 [details] [diff] [review]
part2: remove .groupbox-body dependency in pref dialog

Review of attachment 9012068 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/subdialogs.js
@@ +285,4 @@
>      let groupBoxTitleHeight = groupBoxTitle.clientHeight +
>                                parseFloat(getComputedStyle(groupBoxTitle).borderBottomWidth);
>  
> +    let groupBoxBody = this._box.querySelector("browser");

This code is fragile and I don't believe I was involved in all the calculations. I'd rather not change it. Is the plan to make groupbox-body non-anonymous content? If so, then can we implement that commit first and then just switch this to
> let groupBoxBody = this._box.querySelector(":scope > .groupbox-body");
so there is no behaviour difference.

Otherwise, I agree with Paolo that there is more to cleanup related to the padding.

@@ +289,3 @@
>      // These are deduced from styles which we don't change, so it's safe to get them now:
>      let boxVerticalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingTop);
>      let boxHorizontalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingLeft);

Please submit patches following Mozilla guidelines with more lines of context… 3 is not enough. Using Phabricator would be even better.
Attachment #9012068 - Flags: review?(MattN+bmo)
(Reporter)

Comment 8

7 months ago
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #7)

> > +    let groupBoxBody = this._box.querySelector("browser");
> 
> This code is fragile and I don't believe I was involved in all the
> calculations. I'd rather not change it. Is the plan to make groupbox-body
> non-anonymous content? If so, then can we implement that commit first and
> then just switch this to
> > let groupBoxBody = this._box.querySelector(":scope > .groupbox-body");
> so there is no behaviour difference.

I hoped to get rid of an intermediary vbox, and thus put a browser element there with @flex attribute is on, which should make it of a size of vobx. However, I'm not sure now if can remove that vbox.
(In reply to alexander :surkov (:asurkov) from comment #8)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #7)
> 
> > > +    let groupBoxBody = this._box.querySelector("browser");
> > 
> > This code is fragile and I don't believe I was involved in all the
> > calculations. I'd rather not change it. Is the plan to make groupbox-body
> > non-anonymous content? If so, then can we implement that commit first and
> > then just switch this to
> > > let groupBoxBody = this._box.querySelector(":scope > .groupbox-body");
> > so there is no behaviour difference.
> 
> I hoped to get rid of an intermediary vbox, and thus put a browser element
> there with @flex attribute is on, which should make it of a size of vobx.
> However, I'm not sure now if can remove that vbox.

Yes, I believe for this case there is explicitly intended to be padding on the element (at least, that's what the JS code modified in the patch is measuring). What about adding a groupbox-body box to wrap the <browser> directly in preferences.xul and make sure the existing CSS rules apply (modifying selectors if needed)?
(Reporter)

Comment 10

7 months ago
(In reply to Brian Grinstead [:bgrins] from comment #9)

> Yes, I believe for this case there is explicitly intended to be padding on
> the element (at least, that's what the JS code modified in the patch is
> measuring). What about adding a groupbox-body box to wrap the <browser>
> directly in preferences.xul and make sure the existing CSS rules apply
> (modifying selectors if needed)?

yeah, that what I was going to clarify :) which is:

The weird thing about anonymous .groupbox-body is it has styles applied to it, for example padding [1] and even -moz-appearance on osx [2], which makes harder to get rid of it. It can be deanonymized as the bug summary states, and that'd be an easy fix, but it leads that every groupbox will have to contain vbox/hbox@class="groupbox-body", which looks weird.

So let's wrap every groupbox content by a helper box?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/groupbox.css#18
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/groupbox.css#13
(Assignee)

Comment 11

7 months ago
I guess in most cases we can still move the padding the groupbox element itself, and adjust the caption margins as needed.
(Reporter)

Comment 12

7 months ago
(In reply to :Paolo Amadini from comment #11)
> I guess in most cases we can still move the padding the groupbox element
> itself, and adjust the caption margins as needed.

ok, what about osx -moz-appearance: groupbox style on a groupbox-body element? It may interfere with other element styles if we get rid of .groupbox-body? I can see there's a bunch of elements that don't have -moz-appearance and which are used under groupbox, for example xul:grid element, but wouldn't it be too implicit?
By the way, I asked in #content about using Shadow DOM in chrome (specifically relating to slotting in nodes with XBL into SD slots) and people seemed generally OK with that: https://mozilla.logbot.info/content/20180928#c15388282. So another option here would be to make groupbox a Custom Element with a Shadow Root, then keep the groupbox-body as an anonymous node with a default slot below it.

I think to make that work, we'd have to at least:

1) Figure out if we can reasonably load global.css inside the shadow root (see https://bugzilla.mozilla.org/show_bug.cgi?id=1410579#c5)
2) Figure out how to apply other styles for only certain groupboxes (i.e. in prefs), where we currently rely on piercing the XBL anon content: https://searchfox.org/mozilla-central/search?q=groupbox-body&path=. I guess either use a CSS class or variable on the host element which then gets read from a shadow sheet.

It'd definitely be more work than handling this by rewriting callers to explicitly wrap in .groupbox-body, but this is something we'll need to figure out for a bunch of bindings soon anyway, so maybe worth doing now.
(Reporter)

Comment 14

7 months ago
(In reply to Brian Grinstead [:bgrins] from comment #13)
> By the way, I asked in #content about using Shadow DOM in chrome
> (specifically relating to slotting in nodes with XBL into SD slots) and
> people seemed generally OK with that:
> https://mozilla.logbot.info/content/20180928#c15388282. So another option
> here would be to make groupbox a Custom Element with a Shadow Root, then
> keep the groupbox-body as an anonymous node with a default slot below it.

which means that all elements under <groupbox> have to have @slot attribute, correct? which seems quite the same as requirement to have an explicit <vbox class="groupbox-body"> element (it terms of work scope and their weird look).

> I think to make that work, we'd have to at least:
> 
> 1) Figure out if we can reasonably load global.css inside the shadow root
> (see https://bugzilla.mozilla.org/show_bug.cgi?id=1410579#c5)
> 2) Figure out how to apply other styles for only certain groupboxes (i.e. in
> prefs), where we currently rely on piercing the XBL anon content:
> https://searchfox.org/mozilla-central/search?q=groupbox-body&path=. I guess
> either use a CSS class or variable on the host element which then gets read
> from a shadow sheet.
> 
> It'd definitely be more work than handling this by rewriting callers to
> explicitly wrap in .groupbox-body, but this is something we'll need to
> figure out for a bunch of bindings soon anyway, so maybe worth doing now.

I see the point, but honestly not sure whether <groupbox> wins from @slot mechanism.
(In reply to alexander :surkov (:asurkov) from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > By the way, I asked in #content about using Shadow DOM in chrome
> > (specifically relating to slotting in nodes with XBL into SD slots) and
> > people seemed generally OK with that:
> > https://mozilla.logbot.info/content/20180928#c15388282. So another option
> > here would be to make groupbox a Custom Element with a Shadow Root, then
> > keep the groupbox-body as an anonymous node with a default slot below it.
> 
> which means that all elements under <groupbox> have to have @slot attribute,
> correct? which seems quite the same as requirement to have an explicit <vbox
> class="groupbox-body"> element (it terms of work scope and their weird look).

You can use an default (unnamed) slot for groupbox-body content - that will put children there without them having to opt-in. And then explicitly name the slot for the caption label to the groupbox-title (assuming that caption or a node like it is still going to exist).

So instead of:

    <content>
      <xul:hbox class="groupbox-title">
        <children includes="caption"/>
      </xul:hbox>
      <xul:box class="groupbox-body">
        <children/>
      </xul:box>
    </content>

It'd be something like this in the Shadow DOM:

    <xul:hbox class="groupbox-title">
      <slot name="caption"></slot>
    </xul:hbox>
    <xul:box class="groupbox-body">
      <slot></slot>
    </xul:box>

And then from the calling markup instead of:

    <groupbox>
      <caption label="foo"/>
      <box>...</box>
    </groupbox>

It'd be:

    <groupbox>
      <caption label="foo" slot="caption"/>
      <box>...</box>
    </groupbox>

That said, if you think we can effectively implement groupbox entirely without a Custom Element then that's fine too. I guess there's 3 options:

1) Use CE/SD as outlined above
2) No CE and don't require the groupbox-body to be cloned across all consumers (as Paolo suggested in Comment 11). Clone existing styles on groupbox-body directly to groupbox and figure out the moz-appearance thing in Comment 12. I'm a bit worried this would cause issues with flexing when there are multiple non-caption direct descendants like in https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/components/preferences/colors.xul#37.
3) No CE and always wrap content in the groupbox-body in calling code to get flex and other existing styles to apply.

If (2) worked correctly in all cases that would probably be the best option, but given Comment 12 and the worry I mention above it may not. If all the pieces for SD in chrome were already in place I'd generally prefer (1), but I also don't think (3) is too much of a burden for calling code. What do you think?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(paolo.mozmail)

Updated

7 months ago
Attachment #9011694 - Flags: checkin+
(In reply to Brian Grinstead [:bgrins] from comment #15)
> 2) No CE and don't require the groupbox-body to be cloned across all
> consumers (as Paolo suggested in Comment 11). Clone existing styles on
> groupbox-body directly to groupbox and figure out the moz-appearance thing
> in Comment 12. I'm a bit worried this would cause issues with flexing when
> there are multiple non-caption direct descendants like in
> https://searchfox.org/mozilla-central/rev/
> 819cd31a93fd50b7167979607371878c4d6f18e8/browser/components/preferences/
> colors.xul#37.

Maybe the flex issue could be handled with a rule like `groupbox > *:not(caption) { flex: 1; }` or similar? Not sure if there's a case where that would lead to a different outcome than the current setup.
(Reporter)

Comment 17

7 months ago
It seems the most straightforward way is no CE for groupbox and make <box class="groupbox-body"> explicit (Brian's option 3 from comment 16). No style adjustments, no @slot attributes throughout the content, just wrapping everything by .groupbox-body elements.

Having said that, I'm flexible on the approach. I'm not sure though who should make a call here. Paolo, is it your area?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 18

7 months ago
In most cases, we're not really using the vertical flex, like in Preferences pages and subdialogs, since those are both laid out in continuous mode vertically. In places where it's used, we can easily do without, like in the Page Info dialog. We're also not really using the -moz-appearance in Preferences, which is the major consumer of groupbox:

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/themes/osx/preferences/preferences.css#43

This means we don't need the extra groupbox-body element in most cases - and the patch in Part 2 is probably one of the very few special cases where we need to pay a little bit more attention to the resulting DOM changes.

I'd just say we should go with the first suggestion in bug 1429940 comment 9, which Jared already vetted. We remove "groupbox" and "caption" entirely, including the tag names, and replace them with "vbox" and "description", with classes where needed. There's a few places in the Preferences code that would have to be updated to look for the class name instead of the tag name:

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/components/preferences/in-content/findInPage.js#273
https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/components/preferences/in-content/preferences.js#303

I can morph bug 1429940 to do this for Preferences, and we can either file other bugs or post more patches here for the other consumers. These patches would be reviewed by the peers of each module.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

7 months ago
No longer blocks: 1429940
Depends on: 1429940
(Assignee)

Updated

7 months ago
Summary: replace anonymous groupbox-body by explicit content → Remove "caption" and "groupbox"
(Reporter)

Comment 19

7 months ago
(In reply to :Paolo Amadini from comment #18)
> In most cases, we're not really using the vertical flex, like in Preferences
> pages and subdialogs, since those are both laid out in continuous mode
> vertically. In places where it's used, we can easily do without, like in the
> Page Info dialog. We're also not really using the -moz-appearance in
> Preferences, which is the major consumer of groupbox:
> 
> https://searchfox.org/mozilla-central/rev/
> 819cd31a93fd50b7167979607371878c4d6f18e8/browser/themes/osx/preferences/
> preferences.css#43
> 
> This means we don't need the extra groupbox-body element in most cases - and
> the patch in Part 2 is probably one of the very few special cases where we
> need to pay a little bit more attention to the resulting DOM changes.

yeah, If preferences can manage without .groupbox-body, then it's fine. Otherwise is it ok to keep it explicit for cases where it is required (-moz-appeareance and orientation), correct?

> I'd just say we should go with the first suggestion in bug 1429940 comment
> 9, which Jared already vetted. We remove "groupbox" and "caption" entirely,
> including the tag names, and replace them with "vbox" and "description",
> with classes where needed. There's a few places in the Preferences code that
> would have to be updated to look for the class name instead of the tag name:

vbox/description will break accessibility. You will either need to add ARIA role="group" and @aria-labelledby, or I would suggest to with groupbox/label, that should work for a11y.
(Assignee)

Comment 20

7 months ago
(In reply to alexander :surkov (:asurkov) from comment #19)
> yeah, If preferences can manage without .groupbox-body, then it's fine.
> Otherwise is it ok to keep it explicit for cases where it is required
> (-moz-appeareance and orientation), correct?

These are very few and they won't be using the groupbox-body class. For example, we can probably have a dialog-body class for the Preferences subdialog case. Also, we can probably drop the -moz-appearance everywhere.

I'll move the Preferences question to bug 1429940.
(Assignee)

Updated

7 months ago
Depends on: 1496382
(Assignee)

Updated

7 months ago
Attachment #9012068 - Attachment is obsolete: true
(Assignee)

Comment 21

6 months ago
We're back to this bug :-) Due to the decision to keep the elements without the bindings, and the related changes in dependencies, I'll move here the relevant parts of the patches from bug 1429940 to remove the bindings, and leave the other bug for the accessibility improvements we discussed there.
Blocks: 1429940
Depends on: 1498241
No longer depends on: 1429940
Summary: Remove "caption" and "groupbox" → Remove the "caption" and "groupbox" bindings
(Assignee)

Comment 22

6 months ago
Proper native "groupbox" styling depends on the structure of the XBL binding, but it is now unused except for the Print Page Setup dialog on Windows. The native apperance is thus not applied by default anymore, and the "groupbox" element can just be used semantically for accessibility. The Print Page Setup dialog applies the native styling on its own in a way that still works on Windows.

The only other consumers of "groupbox" are the in-content Preferences pages and dialogs. These are updated to use simpler styles that don't depend on the binding structure.

Depends on D8385
(Assignee)

Comment 24

6 months ago
I was able to test this on Mac, Windows 7, and Windows 10 with and without high contrast mode, and the new groupbox and preferences dialog rendering looks correct.
(Assignee)

Updated

6 months ago
Assignee: surkov.alexander → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1498241
(Assignee)

Comment 28

6 months ago
The Windows 7 failure doesn't look related at all, it may be because of bug 1498803.
(Assignee)

Comment 29

6 months ago
Setting needinfo for review.
Flags: needinfo?(jhofmann)
(Assignee)

Comment 30

6 months ago
Jared, I'm a bit undecided about landing this in 64 versus 65, what do you think? I'm asking you in particular for your opinion on risk of regressions since this mostly affects Preferences, other places are probably fine. We already landed a few other groupbox removals in 64.
Flags: needinfo?(jaws)
Flags: needinfo?(jhofmann)
(Assignee)

Updated

6 months ago
Keywords: leave-open
(Assignee)

Comment 32

6 months ago
For some reason I thought the merge day was further away, but given it's early next week, it probably makes sense to land in 65.
Flags: needinfo?(jaws)
Yeah, seeing this today (on Friday) I would agree with waiting until Monday to land in 65.
(Assignee)

Comment 34

6 months ago
Alex, can you take a look at the test updates in the "accessible" folder in the latest patch revision?
Flags: needinfo?(surkov.alexander)
(Reporter)

Comment 36

6 months ago
(In reply to :Paolo Amadini from comment #34)
> Alex, can you take a look at the test updates in the "accessible" folder in
> the latest patch revision?

done, added few comments in phabricator
Flags: needinfo?(surkov.alexander)

Comment 37

6 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a310eb8e42
Part 2 - Remove the "caption" and "groupbox" bindings. r=bgrins,dao,jaws,johannh,surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4c77d7c5ea
Part 3 - Remove the "prefpane" class and unneeded elements. r=jaws

Comment 38

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42
https://hg.mozilla.org/mozilla-central/rev/ff4c77d7c5ea
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1501755
Perf improvement noticed:

== Change summary for alert #17102 (as of Tue, 23 Oct 2018 22:41:53 GMT) ==

Improvements:

  3%  about_preferences_basic osx-10-10 opt e10s stylo     210.81 -> 205.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17102

Comment 41

6 months ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/4e0ff9553eae33cd88275c5c7c7296e5ae61076f
Port Bug 1493844 - Part 2 - Remove the "caption" and "groupbox" bindings. r=bgrins,dao,jaws,johannh,surkov (#4528)

Comment 42

3 months ago

I suspect this bug changed the styling of the values in the 'security' pane in page info to be not bold anymore. Was that intentional?

Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 43

3 months ago

Yes, it aligns the style with the other panes.

Flags: needinfo?(paolo.mozmail)
No longer depends on: 1535253
Regressions: 1535253
You need to log in before you can comment on or make changes to this bug.