1455433 - Migrate <checkbox> to a Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

Once Bug 1455392 and Bug 1455359 are fixed nsIDOMXULCheckboxElement should only have a `checked` attribute. I believe this will let us replace `<xul:image class="checkbox-check" xbl:inherits="checked,disabled"/>` with a styled HTML input field.

Before we can remove nsIDOMXULCheckboxElement though, we'll have to figure out if the accessibility provided by the HTML element is at parity with nsIDOMXULCheckboxElement (and nsIDOMXULLabeledControlElement which it inherits from).

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: P5 - Feature request for non-primary UI that is not on the roadmap; Code cleanup.

Comment 2

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> Triage: P5 - Feature request for non-primary UI that is not on the roadmap;
> Code cleanup.

Since this bug is part of dexblization project, should you revisit its priority?

Brian, why not to replace xul:checkbox on html:input@type="checkbox"? Is there something weird about

<html:label>Checkbox: <html:input type="checkbox"></html:label> in XUL documents? It would allow to remove the checkbox binging entirely.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #3)
> Brian, why not to replace xul:checkbox on html:input@type="checkbox"? Is
> there something weird about
> 
> <html:label>Checkbox: <html:input type="checkbox"></html:label> in XUL
> documents? It would allow to remove the checkbox binging entirely.

For bindings that have a lot of frontend consumers we've generally attempted to provide a migration to Custom Element that requires as little as possible change to the calling code. If we have something simpler/better we could choose rewrite frontend code instead. Although there can generally be some layout issues when embedding HTML tags with normal CSS layout (flexbox or otherwise) directly next to XUL flexbox elements, so I'd be a little worried about issues with wrapping or overflow when rewriting in this case.

I was actually thinking we'd need to switch to <input type="checkbox"> in order to get rid of nsIDOMXULCheckboxElement, but I see you've already figured that part out in the blocker bugs which is great! So we could potentially use this bug to instead migrate the checkbox XBL binding directly to a Custom Element that creates the exact same markup that the XBL binding uses.

Comment 5

[Brian Grinstead [:bgrins]]

I see checkbox inherits basecontrol (nsIDOMXULControlElement) and basetext (nsIDOMXULLabeledControlElement) so I'd like to be sure we aren't regressing accessibility or anything else by going with the `<html:label>Checkbox: <html:input type="checkbox"></html:label>` approach in Comment 3 or the straight-to-Custom-Element approach in Comment 4.

By the way, we should soon have the ability to implement these interfaces in Custom Elements from Bug 1478372, so if it is necessary to continue to implement them we will be able to.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> I see checkbox inherits basecontrol (nsIDOMXULControlElement) and basetext
> (nsIDOMXULLabeledControlElement) so I'd like to be sure we aren't regressing
> accessibility or anything else by going with the `<html:label>Checkbox:
> <html:input type="checkbox"></html:label>` approach in Comment 3 or the
> straight-to-Custom-Element approach in Comment 4.

no way we can regress accessibility by switching to HTML, HTML is well accessible.

(In reply to Brian Grinstead [:bgrins] from comment #4)

> For bindings that have a lot of frontend consumers we've generally attempted
> to provide a migration to Custom Element that requires as little as possible
> change to the calling code. If we have something simpler/better we could
> choose rewrite frontend code instead. Although there can generally be some
> layout issues when embedding HTML tags with normal CSS layout (flexbox or
> otherwise) directly next to XUL flexbox elements, so I'd be a little worried
> about issues with wrapping or overflow when rewriting in this case.

I see the point. Layout is a concern. I assume display:-moz-box or something may not help.

> I was actually thinking we'd need to switch to <input type="checkbox"> in
> order to get rid of nsIDOMXULCheckboxElement, but I see you've already
> figured that part out in the blocker bugs which is great! So we could
> potentially use this bug to instead migrate the checkbox XBL binding
> directly to a Custom Element that creates the exact same markup that the XBL
> binding uses.

sounds good

Comment 7

[alexander :surkov (:asurkov)]

btw, do you have a good example of XUL widget rewritten into custom controls?

Comment 8

[Brian Grinstead [:bgrins]]

marking ni? to remind myself to answer Comment 7

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #7)
> btw, do you have a good example of XUL widget rewritten into custom controls?

Yeah, the latest one to migrate was <tabbox> in Bug 1469902 but you can see all of the custom elements we currently ship in the scripts loaded at https://searchfox.org/mozilla-central/source/toolkit/content/customElements.js#126-142.

So for the "easy" case, the general workflow is:

1) hg cp toolkit/content/widgets/checkbox.xml toolkit/content/widgets/checkbox.js
2) Add checkbox.js to jar.mn and to the array at https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/content/customElements.js#129
3) Paste in the autogenerated migration at https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Checkbox.js. Note that this usually requires a bit of tweaking to make it work, although I've just pushed up some changes to the script to hopefully limit the amount needed
4) Do some testing and make adjustments as needed.

OK, that's the easy case :). Checkbox is more difficult because:

1) It extends other classes we haven't migrated yet (namely, basecontrol and basetext - see the tree here https://bgrins.github.io/xbl-analysis/tree/)
2) For those base classes, as things are currently we need to be able to implement xpcom interfaces and have c++ be aware of that. Depends on Bug 1478372.
3) We haven't yet gotten rid of <resources>, so the stylesheet loading is still tied to XBL. Depends on Bug 1473921 migrating checkbox.css to a normal stylesheeet. This is fixable, although we'll probably want to wait until after the soft code freeze to do so to avoid potential visual regressions landing late in the cycle. Dão may be able to help us with this after the merge.

So I think we'll need to make some progress on blockers before actual migrating the binding away.

Something I'd like your feedback on Alex is how we can deal with nsIDOMXULControlElement and nsIDOMXULLabeledControlElement. We can wait for (2) above which will let us implement these interfaces in the Custom Elements, but I'd like to ensure that there's not a simpler way forward. Here's what I see so far:

- nsIDOMXULControlElement seems to be used to determine first if a node implements that interface. And then if so, it is used to check if the node is disabled, and if it's an accessKey target: https://searchfox.org/mozilla-central/search?q=nsIDOMXULControlElement&path=. I don't know if there's a better way to determine this, given the wide range of tag names that are under https://bgrins.github.io/xbl-analysis/tree/#basecontrol. There's also some special case for radiogroups where [disabled] is more than just an attr lookup (https://searchfox.org/mozilla-central/source/toolkit/content/widgets/radio.xml#56) but I suppose that can move to c++.
- nsIDOMXULLabeledControlElement seems to be used only by accessibility and then in one special case in JS https://searchfox.org/mozilla-central/search?q=nsIDOMXULLabeledControlElement&path=. For the one case in JS (text.xml) I wonder if we could check for the existence of bindingParent.accessKey instead. I'd like your opinion on if/how we could handle the accessibility cases if not by XPCOM.

Comment 10

[alexander :surkov (:asurkov)]

Thank you for the pointers, they are quite helpful.

It appears we can kill nsIDOMXULControlElement by replacing it by c++ XULElement::IsControl() method, and also checking DOM attribute directly instead 'disabled' property.

nsIDOMXULLabeledControlElement looks a bit more complicated since as you said it also used in JS, but I guess bindingParent.accessKey will work just fine in that case.

Comment 11

[Brian Grinstead [:bgrins]]

A few notes:

1) I've got an implementation of MozBaseText for <radio> at https://phabricator.services.mozilla.com/D8121 (see customElements.js). You may well get to this before I do for <radio> so feel free to grab that for your patch.
2) In that same patch I have a little helper function I'm using to simulate the [implements] functionality. It's probably worth unifying on something here and exposing it on MozXULElement since a bunch of other XBL content relies on it. I'd be interested in how you think we can handle that.
3) Please do some talos pushes with this. You'll probably at least want to use `if (this.delayConnectedCallback()) { return; }` at the top of the connectedCallback (added in Bug 1496425), so we don't end up creating DOM during initial parse.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> A few notes:
> 
> 1) I've got an implementation of MozBaseText for <radio> at
> https://phabricator.services.mozilla.com/D8121 (see customElements.js). You
> may well get to this before I do for <radio> so feel free to grab that for
> your patch.
> 2) In that same patch I have a little helper function I'm using to simulate
> the [implements] functionality. It's probably worth unifying on something
> here and exposing it on MozXULElement since a bunch of other XBL content
> relies on it. I'd be interested in how you think we can handle that.

Did you mean inherits here?

> 3) Please do some talos pushes with this. You'll probably at least want to
> use `if (this.delayConnectedCallback()) { return; }` at the top of the
> connectedCallback (added in Bug 1496425), so we don't end up creating DOM
> during initial parse.

sure

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #12)
> > 2) In that same patch I have a little helper function I'm using to simulate
> > the [implements] functionality. It's probably worth unifying on something
> > here and exposing it on MozXULElement since a bunch of other XBL content
> > relies on it. I'd be interested in how you think we can handle that.
> 
> Did you mean inherits here?

Yeah - that's what I meant to say.

Comment 14

[alexander :surkov (:asurkov)]

I've run into the issue: if checkbox content is explicit, then bunch of tests fails, because they find unexpected labels in a document etc, tests can be fixed for sure, but other browser parts may also have a dependency on it, so it's kinda worrisome. I tried to switch to shadow DOM, but stuck into styling problem. All styles are externals currently, thus I could import common.css etc into shadow DOM for sure (or move them into checkbox.css), but there's bunch of styling bits outside the toolkit module. What your thinking would be on this?

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #14)
> I've run into the issue: if checkbox content is explicit, then bunch of
> tests fails, because they find unexpected labels in a document etc, tests
> can be fixed for sure, but other browser parts may also have a dependency on
> it, so it's kinda worrisome. I tried to switch to shadow DOM, but stuck into
> styling problem. All styles are externals currently, thus I could import
> common.css etc into shadow DOM for sure (or move them into checkbox.css),
> but there's bunch of styling bits outside the toolkit module. What your
> thinking would be on this?

Can you post a patch with and without Shadow DOM, and a pointer to a test that's failing without it (or a try push)?

Having extra DOM beneath a checkbox seems generally OK to me in this case. But as you point out using Shadow DOM would fit in better with existing code. And we're going to have to figure out these styling issues one way or another, so it's worth at least thinking about that here as well. If you have a patch that uses SD I'd like to pull it down to play around with it.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: wip

shadow DOM approach: styling issue plus number of assertions, Node_Binding::appendChild is on stack. It has to be some issue with shadow DOM.

[1073, Main Thread] ###!!! ASSERTION: Element not in doc!: 'aContent->GetUncomposedDoc() == this', file /builds/worker/workspace/build/src/dom/xul/XULDocument.cpp, line 961
[task 2018-10-11T02:40:55.505Z] 02:40:55     INFO - GECKO(1073) | #01: mozilla::dom::XULDocument::AddSubtreeToDocument(nsIContent*) [dom/xul/XULDocument.cpp:961]

I don't have no shadow DOM at hands, but it can be easily converted by replacing |this.shadowRoot| by |this| and removing this.attachShadow() bit. I think this test was failing [1], because it queries all labels within a pref page, which obviously includes the one from checkbox explicit content.

[1] browser/components/preferences/in-content/tests/browser_extension_controlled.js

Comment 17

[alexander :surkov (:asurkov)]

it looks like :host doesn't work in XUL environment. I tried:

<style xmlns="http://www.w3.org/1999/xhtml">
  *|*:host {
    background-color: green;
  }
</style>`;

<style xmlns="http://www.w3.org/1999/xhtml">
  @import url("chrome://global/skin/checkbox.css");
</style>

<link xmlns="http://www.w3.org/1999/xhtml" rel="stylesheet"
      href="chrome://global/skin/checkbox.css" />

this.shadowRoot.appendChild(MozXULElement.parseXULToFragment(content));

:host styles seems never applied to a checkbox

however web-platform test seems working, see [1] and [2], thus I suppose there's some XUL related issue. Please let me know if I miss something.

I can see two ways to fix this bug, assuming :host problem cannot be resolved:
* move checkbox tag name styles into common.css or whatever place suits the best
* process with explicit content

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-scoping/shadow-link-rel-stylesheet.html#9
[2] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-scoping/shadow-at-import.html#10

Comment 18

[:Paolo Amadini]

In general, for final patches I would suggest not using Shadow DOM with Custom Elements when fixing tests accordingly is not too much work.

Of course, it's still interesting to use simpler elements like this ones as test cases for identifying bugs or limitations like the ones you found here, even if the final patch will be different.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: wip: shadow DOM approach

ok, then putting my latest patch with shadow DOM approach just in case

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch: explicit approach

Comment 21

[:Paolo Amadini]

Comment on attachment 9019001 [details] [diff] [review]
patch: explicit approach

We're still discussing how to do inheritance, clearing review for now. Also, probably we'll want to keep checkbox.js and radio.js as similar as we can.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #21)
> Comment on attachment 9019001 [details] [diff] [review]
> patch: explicit approach
> 
> We're still discussing how to do inheritance, clearing review for now. Also,
> probably we'll want to keep checkbox.js and radio.js as similar as we can.

where does the discussion happens? I think I'm good with any working solution, but curious about cons and pros of the discussed approaches and this one.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #22)
> (In reply to :Paolo Amadini from comment #21)
> > Comment on attachment 9019001 [details] [diff] [review]
> > patch: explicit approach
> > 
> > We're still discussing how to do inheritance, clearing review for now. Also,
> > probably we'll want to keep checkbox.js and radio.js as similar as we can.
> 
> where does the discussion happens? I think I'm good with any working
> solution, but curious about cons and pros of the discussed approaches and
> this one.

I think this is referring to https://phabricator.services.mozilla.com/D9322#222688.

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: Bug 1455433 - WIP - implement inheritAttribute(s) helper functions on MozXULElement

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Created attachment 9020163 [details]
> Bug 1455433 - WIP - implement inheritAttribute(s) helper functions on
> MozXULElement

I think like XBL-like syntax more than JS-like syntax from my patch, but I would prefer to move observedAttributes and attributeChangedCallback into base class to keep API straightforward, if there's no any issues with inheritance when statics are involved.

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 9019001 [details] [diff] [review]
patch: explicit approach

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

::: toolkit/content/customElements.js
@@ +64,5 @@
> +    this.textContent = v;
> +  }
> +
> +  static get observedAttributes() {
> +    return this.attrInherits;

This won't work - observedAttributes needs to be static and fetchable when customElements.define is called. It can't read instance properties.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 9019001 [details] [diff] [review]
patch: explicit approach

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

::: toolkit/content/widgets/checkbox.js
@@ +42,5 @@
> +      </hbox>
> +    `;
> +    let contentFragment = MozXULElement.parseXULToFragment(content);
> +
> +    let labelEl = contentFragment.querySelector(".checkbox-label");

Make sure you access labelEl from `this` after it's appended into the host. We have to wait to make a JS reference to the node until it's in the DOM because of XBL - see this comment: https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/toolkit/content/customElements.js#63-88

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #25)
> (In reply to Brian Grinstead [:bgrins] from comment #24)
> > Created attachment 9020163 [details]
> > Bug 1455433 - WIP - implement inheritAttribute(s) helper functions on
> > MozXULElement
> 
> I think like XBL-like syntax more than JS-like syntax from my patch, but I
> would prefer to move observedAttributes and attributeChangedCallback into
> base class to keep API straightforward, if there's no any issues with
> inheritance when statics are involved.

See the discussion in the phab pushes around this (I'm going to leave a response there to avoid fragmenting the discussion).

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #26)

> > +
> > +  static get observedAttributes() {
> > +    return this.attrInherits;
> 
> This won't work - observedAttributes needs to be static and fetchable when
> customElements.define is called. It can't read instance properties.

I tried this example:

class BaseClass {
  static foo() { console.log('base'); }
};

class DerivedClass extends BaseClass {
  static foo() { console.log('derived'); }
};

class Derived2Class extends BaseClass {
  
};

BaseClass.foo();
DerivedClass.foo();
Derived2Class.foo();

It gives output:

base
derived
base

Are customElements.define dependencies are somewhat different?

Comment 30

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #29)
> (In reply to Brian Grinstead [:bgrins] from comment #26)
> 
> > > +
> > > +  static get observedAttributes() {
> > > +    return this.attrInherits;
> > 
> > This won't work - observedAttributes needs to be static and fetchable when
> > customElements.define is called. It can't read instance properties.
> 
> I tried this example:

I think I missed fetchable part, so nm

Comment 31

[alexander :surkov (:asurkov)]

Attachment: inheritAttributes helper

Brian's patch

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 9020261 [details] [diff] [review]
inheritAttributes helper

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

I filed Bug 1502947 since this can land separately, and we want to use it in a number of other bugs. Can you please move the patch there?

::: toolkit/content/customElements.js
@@ +49,5 @@
> +   *        attribute, defined a child element. Note |from| may
> +   *        take a special value of "text" to propogate attribute value as
> +   *        a child's text.
> +   */
> +  inheritAttributes(child, attrs) {

I pinged Paolo about this, and he'd prefer to use the 'singular' version of this API (`inheritAttribute`) and loop through attrs from consumers. Then we would handle the plural case in the future, more declarative API.

So the caller would do something like:

for (let attr of ["text=label", "foo", "boo", "bardo=bar"]) {
  this.inheritAttribute(this.label, attr);
}

or if we only have a single one to do:

this.inheritAttribute(this.label, "foo");

Comment 33

[Brian Grinstead [:bgrins]]

Did you end up seeing any talos regressions? And, did the tests you mention in Comment 14 get fixed with the 'explicit approach' patch?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #33)
> Did you end up seeing any talos regressions?

I don't recall any regression, but let me rebase it against bug 1502947 and re-run the tests

> And, did the tests you mention
> in Comment 14 get fixed with the 'explicit approach' patch?

I adjusted the test

Comment 35

[alexander :surkov (:asurkov)]

Attachment: patch

rebased version

Comment 36

[:Paolo Amadini]

I don't think I'll be able to get to this review this week, but will try to do it early next week, unless Brian wants to review it before.

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 9021401 [details] [diff] [review]
patch

is seeing failing tests, looking

Comment 38

[Brian Grinstead [:bgrins]]

Feel free to flag me for review once you get comment 37 sorted out.

Comment 39

[alexander :surkov (:asurkov)]

Cannot figure out what's wrong with reftest toolkit/themes/osx/reftests/checkboxsize.xul. It complains that "FAIL image comparison, max difference: 198, number of differing pixels: 956". I cannot spot a difference visually when loading checkboxsize.xul and checkboxsize-ref.xul manually.

Failing log is:

 0:13.85 TEST_START: file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize.xul == file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize-ref.xul
 0:13.86 GECKO(47898) REFTEST TEST-LOAD | file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize.xul | 0 / 1 (0%)
 0:14.03 INFO drawWindow flags = DRAWWINDOW_DRAW_CARET | DRAWWINDOW_DRAW_VIEW | DRAWWINDOW_USE_WIDGET_LAYERS; window size = 800,1000; test browser size = 800,1000
 0:14.10 GECKO(47898) REFTEST TEST-LOAD | file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize-ref.xul | 0 / 1 (0%)
 0:14.26 FAIL image comparison, max difference: 198, number of differing pixels: 956
 0:14.26 INFO Saved log: START file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize.xul
 0:14.26 INFO Saved log: [CONTENT] Using browser remote=true

 0:14.26 INFO Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
 0:14.26 INFO Saved log: Initializing canvas snapshot
 0:14.26 INFO Saved log: DoDrawWindow 0,0,800,1000
 0:14.26 INFO Saved log: [CONTENT] RecordResult fired
 0:14.26 INFO Saved log: RecordResult fired
 0:14.26 INFO Saved log: START file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize-ref.xul
 0:14.26 INFO Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
 0:14.26 INFO Saved log: Initializing canvas snapshot
 0:14.26 INFO Saved log: DoDrawWindow 0,0,800,1000
 0:14.26 INFO Saved log: [CONTENT] RecordResult fired
 0:14.26 INFO Saved log: RecordResult fired
 0:14.26 TEST_END: Test OK. Subtests passed 0/1. Unexpected 1

Correct log:

 0:14.85 TEST_START: file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize.xul == file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize-ref.xul
 0:14.86 GECKO(43572) REFTEST TEST-LOAD | file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize.xul | 0 / 1 (0%)
 0:15.02 INFO drawWindow flags = DRAWWINDOW_DRAW_CARET | DRAWWINDOW_DRAW_VIEW | DRAWWINDOW_USE_WIDGET_LAYERS; window size = 800,1000; test browser size = 800,1000
 0:15.10 GECKO(43572) REFTEST TEST-LOAD | file:///Users/heh/mozilla/inbound/toolkit/themes/osx/reftests/checkboxsize-ref.xul | 0 / 1 (0%)
 0:15.17 PASS image comparison, max difference: 0, number of differing pixels: 0
 0:15.17 TEST_END: Test OK. Subtests passed 1/1. Unexpected 0

Any pointers where to look at?

Comment 40

[alexander :surkov (:asurkov)]

Marcus, do you have ideas on comment #39?

Comment 41

[Dão Gottwald [:dao]]

(In reply to alexander :surkov (:asurkov) from comment #39)
> Cannot figure out what's wrong with reftest
> toolkit/themes/osx/reftests/checkboxsize.xul. It complains that "FAIL image
> comparison, max difference: 198, number of differing pixels: 956". I cannot
> spot a difference visually when loading checkboxsize.xul and
> checkboxsize-ref.xul manually.

Maybe the screenshot is taken before the custom element stuff is applied?

Comment 42

[alexander :surkov (:asurkov)]

(In reply to Dão Gottwald [::dao] from comment #41)
> (In reply to alexander :surkov (:asurkov) from comment #39)
> > Cannot figure out what's wrong with reftest
> > toolkit/themes/osx/reftests/checkboxsize.xul. It complains that "FAIL image
> > comparison, max difference: 198, number of differing pixels: 956". I cannot
> > spot a difference visually when loading checkboxsize.xul and
> > checkboxsize-ref.xul manually.
> 
> Maybe the screenshot is taken before the custom element stuff is applied?

do you know place in code where screenhot is taken? I know about nothing of reftests implementation

Comment 43

[Dão Gottwald [:dao]]

Don't know offhand, but you can set class="reftest-wait" on the window element and remove it onload or whenever you want.

Comment 44

[Markus Stange [:mstange]]

(In reply to alexander :surkov (:asurkov) from comment #39)
> Any pointers where to look at?

You omitted the most important two lines from the log you pasted: the data URLs that contain the images that were compared.
You can paste an excerpt from your log into https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml and inspect the differences visually. If this is failing on a try push, treeherder even gives you a button to open the reftest analyzer for the failing job. (The button has a bar chart icon and is located in the lower left corner, next to the retrigger button.)

Comment 45

[alexander :surkov (:asurkov)]

(In reply to Markus Stange [:mstange] from comment #44)
> (In reply to alexander :surkov (:asurkov) from comment #39)
> > Any pointers where to look at?
> 
> You omitted the most important two lines from the log you pasted: the data
> URLs that contain the images that were compared.
> You can paste an excerpt from your log into
> https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> reftest-analyzer.xhtml and inspect the differences visually. If this is
> failing on a try push, treeherder even gives you a button to open the
> reftest analyzer for the failing job. (The button has a bar chart icon and
> is located in the lower left corner, next to the retrigger button.)

thank you, here's a link to a try server analyzer

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/NVmry8pnQgaqmpSAfqobOg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

It appears that reftest image has darker color. There's no image on the right to move over it. I wonder, if it means that a checkbox is not drawing at all?

Comment 46

[alexander :surkov (:asurkov)]

ni? Markus for comment 45 ^

Comment 47

[Markus Stange [:mstange]]

(In reply to alexander :surkov (:asurkov) from comment #45)
> I wonder, if it means that a checkbox is not drawing at all?

That's right: If you select "Image 1" at the top, you can see only a gray background, and if you select "Image 2", you'll see a gray background with checkboxes on it. So that means that checkboxsize.xul does not have any checkboxes at the time the reftest snapshot is taken, but checkboxsize-ref.xul does have checkboxes at the time the snapshot is taken.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to Markus Stange [:mstange] from comment #47)
> (In reply to alexander :surkov (:asurkov) from comment #45)
> > I wonder, if it means that a checkbox is not drawing at all?
> 
> That's right: If you select "Image 1" at the top, you can see only a gray
> background, and if you select "Image 2", you'll see a gray background with
> checkboxes on it. So that means that checkboxsize.xul does not have any
> checkboxes at the time the reftest snapshot is taken, but
> checkboxsize-ref.xul does have checkboxes at the time the snapshot is taken.

thank you, Markus. So checkbox custom element is not yet loaded when the reftest is running. Curious, whether this is a timing issue or customElements.js is not loaded into that file for some reason.

Comment 49

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #48)
> (In reply to Markus Stange [:mstange] from comment #47)
> > (In reply to alexander :surkov (:asurkov) from comment #45)
> > > I wonder, if it means that a checkbox is not drawing at all?
> > 
> > That's right: If you select "Image 1" at the top, you can see only a gray
> > background, and if you select "Image 2", you'll see a gray background with
> > checkboxes on it. So that means that checkboxsize.xul does not have any
> > checkboxes at the time the reftest snapshot is taken, but
> > checkboxsize-ref.xul does have checkboxes at the time the snapshot is taken.
> 
> thank you, Markus. So checkbox custom element is not yet loaded when the
> reftest is running. Curious, whether this is a timing issue or
> customElements.js is not loaded into that file for some reason.

Now that you mention it, I bet this is Bug 1465457.

Comment 50

[Brian Grinstead [:bgrins]]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1465457#c11, it sounds like there's a way to load reftests in a mochitest which might unblock this particular case. But I'm guessing we'll keep seeing this come up and we should probably take a decision on switching them all to run in chrome context.

Comment 51

[Dão Gottwald [:dao]]

(In reply to alexander :surkov (:asurkov) from comment #48)
> (In reply to Markus Stange [:mstange] from comment #47)
> > (In reply to alexander :surkov (:asurkov) from comment #45)
> > > I wonder, if it means that a checkbox is not drawing at all?
> > 
> > That's right: If you select "Image 1" at the top, you can see only a gray
> > background, and if you select "Image 2", you'll see a gray background with
> > checkboxes on it. So that means that checkboxsize.xul does not have any
> > checkboxes at the time the reftest snapshot is taken, but
> > checkboxsize-ref.xul does have checkboxes at the time the snapshot is taken.
> 
> thank you, Markus. So checkbox custom element is not yet loaded when the
> reftest is running. Curious, whether this is a timing issue or
> customElements.js is not loaded into that file for some reason.

You can test this by using reftest-wait as mentioned earlier.

Comment 52

[alexander :surkov (:asurkov)]

Attachment: Bug 1455433 - migrate xul:checkbox to CE, r=bgrins

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/623c6ca6ed6c
migrate xul:checkbox to CE, r=bgrins

Comment 54

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/623c6ca6ed6c

Comment 55

[Onno Ekker [:nONoNonO UTC+1]]

In Tools > Preferences on labels of checkboxes, the accesskey's aren't visible anymore. I noticed this in Thunderbird 67.0a1, but the same holds for Firefox 67.0a1...

Comment 56

[Magnus Melin [:mkmelin]]

Filed that as bug 1536059 - Clone This Bug is just a few more clicks. Usually best to file new bugs for any follow-up work.

Comment 57

[alexander :surkov (:asurkov)]

Brian, how do you feel about backing out the patch from 67, leaving it on 68? It should give enough time to sort out all issues.

Comment 58

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #57)

Brian, how do you feel about backing out the patch from 67, leaving it on 68? It should give enough time to sort out all issues.

At this point, uplifting a backout feels more risky than uplifting the trivial fix for accesskeys in Bug 1536059. The only other open blocker here is the osx about:preferences#privacy regression and there are a few ideas for that on the bug.