Closed Bug 1451400 Opened 2 years ago Closed 2 years ago

Migrate dropmarker from XBL to a Custom Element

Categories

(Firefox :: General, task, P3)

task

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: bgrins)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Blocks: war-on-xbl
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
(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
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.
Attachment #8964979 - Attachment is obsolete: true
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 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+
(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
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
Summary: Avoid extending xul:button in the dropmarker bindings. → Migrate dropmarker from XBL to a Custom Element
https://hg.mozilla.org/mozilla-central/rev/524cb5df3a4b
https://hg.mozilla.org/mozilla-central/rev/ffe61cbaa084
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1478999
Depends on: 1485337
Type: enhancement → task
Blocks: 1560556
You need to log in before you can comment on or make changes to this bug.