Closed Bug 1455433 Opened 2 years ago Closed 1 year ago

Migrate <checkbox> to a Custom Element

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: bgrins, Assigned: surkov)

References

(Regressed 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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).
Triage: P5 - Feature request for non-primary UI that is not on the roadmap; Code cleanup.
Priority: -- → P5
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Depends on: 1486671
Depends on: 1486674
(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.
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
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.
(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
btw, do you have a good example of XUL widget rewritten into custom controls?
marking ni? to remind myself to answer Comment 7
Flags: needinfo?(bgrinstead)
Summary: Use an HTML input for XUL <checkbox> → Migrate <checkbox> to a Custom Element
Depends on: 1478372
Depends on: 1473921
(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.
Flags: needinfo?(bgrinstead) → needinfo?(surkov.alexander)
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.
Flags: needinfo?(surkov.alexander)
Depends on: 1491210
Depends on: 1492046
Assignee: nobody → surkov.alexander
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.
See Also: → 1495861
(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
(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.
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?
(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.
Attached patch wip (obsolete) — Splinter Review
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
Attachment #9016496 - Attachment is patch: true
Attachment #9016496 - Attachment mime type: application/octet-stream → text/plain
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
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.
ok, then putting my latest patch with shadow DOM approach just in case
Attachment #9016496 - Attachment is obsolete: true
Attachment #9018944 - Attachment is patch: true
Attachment #9018944 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch: explicit approach (obsolete) — Splinter Review
Attachment #9019001 - Flags: review?(paolo.mozmail)
Attachment #9019001 - Attachment is patch: true
Attachment #9019001 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #9019001 - Flags: review?(paolo.mozmail)
(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.
(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.
(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 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 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
(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).
(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?
(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
Attached patch inheritAttributes helper (obsolete) — Splinter Review
Brian's patch
Attachment #9020261 - Flags: review?(paolo.mozmail)
Depends on: 1502947
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");
Attachment #9020261 - Flags: review?(paolo.mozmail)
Attachment #9020163 - Attachment is obsolete: true
Did you end up seeing any talos regressions? And, did the tests you mention in Comment 14 get fixed with the 'explicit approach' patch?
Flags: needinfo?(surkov.alexander)
(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
Flags: needinfo?(surkov.alexander)
Attached patch patchSplinter Review
rebased version
Attachment #9019001 - Attachment is obsolete: true
Attachment #9020261 - Attachment is obsolete: true
Attachment #9021401 - Flags: review?(paolo.mozmail)
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 on attachment 9021401 [details] [diff] [review]
patch

is seeing failing tests, looking
Attachment #9021401 - Flags: review?(paolo.mozmail)
Feel free to flag me for review once you get comment 37 sorted out.
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?
Marcus, do you have ideas on comment #39?
Flags: needinfo?(mstange)
(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?
(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
Don't know offhand, but you can set class="reftest-wait" on the window element and remove it onload or whenever you want.
(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.)
Flags: needinfo?(mstange)
(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?
ni? Markus for comment 45 ^
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(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.
(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.
Depends on: 1465457
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.
(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.
See Also: → 1523429
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/623c6ca6ed6c
migrate xul:checkbox to CE, r=bgrins
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Depends on: 1536059

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.

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.

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

No longer depends on: 1541045
Regressions: 1541045
Type: enhancement → task
Component: XUL → XUL Widgets
Product: Core → Toolkit
Regressions: 1539414
No longer depends on: 1532651
Regressions: 1532651
You need to log in before you can comment on or make changes to this bug.