1493844 - Remove the "caption" and "groupbox" bindings

Status: RESOLVED FIXED

(Core :: XUL, task, P1)

Description

[alexander :surkov (:asurkov)]

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.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: part1: remove unused style for pageInfo

it appears it is artifact and no longer used, kill it

Comment 2

[:Paolo Amadini]

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f4bc25eec8
part1, remove unused style for pageInfo, r=paolo

Comment 4

[alexander :surkov (:asurkov)]

Attachment: part2: remove .groupbox-body dependency in pref dialog

Comment 5

[:Paolo Amadini]

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.

Comment 6

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/83f4bc25eec8

Comment 7

[Matthew N. [:MattN]]

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.

Comment 8

[alexander :surkov (:asurkov)]

(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.

Comment 9

[Brian Grinstead [:bgrins]]

(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)?

Comment 10

[alexander :surkov (:asurkov)]

(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

Comment 11

[:Paolo Amadini]

I guess in most cases we can still move the padding the groupbox element itself, and adjust the caption margins as needed.

Comment 12

[alexander :surkov (:asurkov)]

(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?

Comment 13

[Brian Grinstead [:bgrins]]

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.

Comment 14

[alexander :surkov (:asurkov)]

(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.

Comment 15

[Brian Grinstead [:bgrins]]

(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?

Comment 16

[Brian Grinstead [:bgrins]]

(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.

Comment 17

[alexander :surkov (:asurkov)]

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?

Comment 18

[:Paolo Amadini]

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.

Comment 19

[alexander :surkov (:asurkov)]

(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.

Comment 20

[:Paolo Amadini]

(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.

Comment 21

[:Paolo Amadini]

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.

Comment 22

[:Paolo Amadini]

Attachment: Bug 1493844 - Part 2 - Remove the "caption" and "groupbox" bindings. r=bgrins,dao

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

Comment 23

[:Paolo Amadini]

Attachment: Bug 1493844 - Part 3 - Remove the "prefpane" class and unneeded elements. r=jaws

Depends on D7581

Comment 24

[:Paolo Amadini]

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.

Comment 25

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b47ba4d9e945b3d7d89e30b7ece3a58aa4b1a5b

Comment 27

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eb520a2c4733f4bc571c557f2825b86b4cc7a3e

Comment 28

[:Paolo Amadini]

The Windows 7 failure doesn't look related at all, it may be because of bug 1498803.

Comment 29

[:Paolo Amadini]

Setting needinfo for review.

Comment 30

[:Paolo Amadini]

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.

Comment 31

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0617319841b075d8660585cc6eea31dc7337986c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba6235fb85ae43d2ac7319f52834e6483cec8f7

Comment 32

[:Paolo Amadini]

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.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, seeing this today (on Friday) I would agree with waiting until Monday to land in 65.

Comment 34

[:Paolo Amadini]

Alex, can you take a look at the test updates in the "accessible" folder in the latest patch revision?

Comment 35

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15a125397b8d73ce5af3eabee87334d09a6aab16

Comment 36

[alexander :surkov (:asurkov)]

(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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42
https://hg.mozilla.org/mozilla-central/rev/ff4c77d7c5ea

Comment 39

[Ionuț Goldan [:igoldan]]

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 40

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4528

Comment 41

[[github robot]]

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

[:Gijs (he/him)]

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?

Comment 43

[:Paolo Amadini]

Yes, it aligns the style with the other panes.