Migrate dropmarker from XBL to a Custom Element

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: emilio, Assigned: bgrins)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Blocks: 1397874
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8964979 [details]
Bug 1451400: Avoid extending xul:button in the dropmarker bindings.

https://reviewboard.mozilla.org/r/233714/#review239386

::: commit-message-6ddfc:4
(Diff revision 1)
> +Bug 1451400: Avoid extending xul:button in the dropmarker bindings. r?dao
> +
> +Similarly to toolbarpaletteitem, this only seems relevant to avoid clicking in
> +the icon from capturing the event.

<toolbarpaletteitem> can have arbitrary content, <dropmarker> cannot, so... what exactly is the point of this? What events would this <image> consume?
Attachment #8964979 - Flags: review?(dao+bmo)
Priority: -- → P3
(Assignee)

Comment 3

10 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> Comment on attachment 8964979 [details]
> Bug 1451400: Avoid extending xul:button in the dropmarker bindings.
> 
> https://reviewboard.mozilla.org/r/233714/#review239386
> 
> ::: commit-message-6ddfc:4
> (Diff revision 1)
> > +Bug 1451400: Avoid extending xul:button in the dropmarker bindings. r?dao
> > +
> > +Similarly to toolbarpaletteitem, this only seems relevant to avoid clicking in
> > +the icon from capturing the event.
> 
> <toolbarpaletteitem> can have arbitrary content, <dropmarker> cannot, so...
> what exactly is the point of this? What events would this <image> consume?

So maybe we don't need the `extends` at all? I can put up a patch that converts dropmarker to a Custom Element and drops the attribute altogether.
Assignee: emilio → bgrinstead
(Assignee)

Comment 4

10 months ago
Actually, we do need to set pointer-events on the image after all. Otherwise clicking on the dropmarker in the urlbar breaks. I'm guessing we have JS checking if the event target is the dropmarker but without extends/pointer-events it's the dropmarker-icon image instead.
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8964979 - Attachment is obsolete: true

Comment 7

10 months ago
mozreview-review
Comment on attachment 8975914 [details]
Bug 1451400 - Part 1 - Load dropmarker.css in components.css instead of with XBL;

https://reviewboard.mozilla.org/r/244122/#review250708
Attachment #8975914 - Flags: review?(dao+bmo) → review+

Comment 8

10 months ago
mozreview-review
Comment on attachment 8975915 [details]
Bug 1451400 - Part 2 - Migrate dropmarker XBL to a Custom Element;

https://reviewboard.mozilla.org/r/244124/#review250710

::: toolkit/content/widgets/general.js:41
(Diff revision 1)
>  customElements.define("deck", MozDeck);
>  
> +class MozDropmarker extends XULElement {
> +  connectedCallback() {
> +    // Only create the image the first time we are connected
> +    if (!this.querySelector(".dropmarker-icon")) {

if (!this.firstChild) is cheaper and would probably be good enough?
Attachment #8975915 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 9

10 months ago
(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8975915 [details]
> Bug 1451400 - Part 2 - Migrate dropmarker XBL to a Custom Element;
> 
> https://reviewboard.mozilla.org/r/244124/#review250710
> 
> ::: toolkit/content/widgets/general.js:41
> (Diff revision 1)
> >  customElements.define("deck", MozDeck);
> >  
> > +class MozDropmarker extends XULElement {
> > +  connectedCallback() {
> > +    // Only create the image the first time we are connected
> > +    if (!this.querySelector(".dropmarker-icon")) {
> 
> if (!this.firstChild) is cheaper and would probably be good enough?

Yep - will update
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

10 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/524cb5df3a4b
Part 1 - Load dropmarker.css in components.css instead of with XBL;r=dao
https://hg.mozilla.org/integration/autoland/rev/ffe61cbaa084
Part 2 - Migrate dropmarker XBL to a Custom Element;r=dao
(Assignee)

Updated

10 months ago
Summary: Avoid extending xul:button in the dropmarker bindings. → Migrate dropmarker from XBL to a Custom Element

Comment 13

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/524cb5df3a4b
https://hg.mozilla.org/mozilla-central/rev/ffe61cbaa084
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1478999

Updated

7 months ago
Depends on: 1485337
You need to log in before you can comment on or make changes to this bug.