1451400 - Migrate dropmarker from XBL to a Custom Element

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1451400: Avoid extending xul:button in the dropmarker bindings.

Similarly to toolbarpaletteitem, this only seems relevant to avoid clicking in
the icon from capturing the event.

I didn't see any other rendering difference on the dropmarkers I know (the
history one, and the ones in buttons), but I could only test Linux so I'd
appreciate a sanity-check on other OSes.

I used inline style because I wasn't really enthusiastic about repeating the
same rule three times in dropmarker.css instead.

Review commit: https://reviewboard.mozilla.org/r/233714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233714/

Comment 2

[Dão Gottwald [:dao]]

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?

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Brian Grinstead [:bgrins]]

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 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1451400 - Part 1 - Load dropmarker.css in components.css instead of with XBL;

Review commit: https://reviewboard.mozilla.org/r/244122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244122/

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1451400 - Part 2 - Migrate dropmarker XBL to a Custom Element;

Review commit: https://reviewboard.mozilla.org/r/244124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244124/

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Dão Gottwald [:dao]]

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?

Comment 9

[Brian Grinstead [:bgrins]]

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

[Brian Grinstead [:bgrins]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244122/diff/1-2/

Comment 11

[Brian Grinstead [:bgrins]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244124/diff/1-2/

Comment 12

[Pulsebot]

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

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/524cb5df3a4b
https://hg.mozilla.org/mozilla-central/rev/ffe61cbaa084