Remove "image" XBL binding

RESOLVED FIXED in Firefox 58

Status

()

P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: Paolo)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [xbl-special-cases])

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
We attach the "image" binding to XUL <image> tags: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/general.xml#157.  It only adds a single property on top of nsIDOMXULImageElement (src).

We should do a prototype that removes the binding entirely and moves the property into C++, as an alternative to migrating it to a Custom Element.
(Reporter)

Updated

2 years ago
Priority: -- → P5
(Reporter)

Comment 1

2 years ago
Boris, I had two questions about this:

1) Are there any functional differences between XUL <image> and HTML <img>? I'm trying to figure out if it would make more sense to find/replace <xul:image> with <html:img> rather than attempting to remove the image XBL binding
2) How could I go about adding 'src' as a property to XUL <image> tags without using the XBL `<implementation implements="nsIDOMXULImageElement">`?
Flags: needinfo?(bzbarsky)
> 1) Are there any functional differences between XUL <image> and HTML <img>?

Oh, yes.

1) With HTML <img>, the node owns the image load.  The layout object just does sizing and painting.

With XUL <image>, the node doesn't own the image load, because we only have one XULElement class.  So the layout object (nsImageBoxFrame) owns the image load.  They have very little code in common, actually.

As a concrete example of the difference, if you take a <xul:image> display:none and then bring it back, you will get a new "load" event fired.  That does not happen with <html:img>.  On the other hand, an HTML <img> exposes all sorts of information about the image (naturalWidth, etc) that <xul:image> doesn't expose.

2) <xul:image> will use the "list-style-image" CSS property to get the relevant image url.  In that case it will also use the -moz-image-region property to only render part of that image, if set.  So we have styles like this (in pocket.css, but we have a few hundred other uses of moz-image-region):

  toolbarpaletteitem[place="palette"] > #pocket-button {
    list-style-image: url(chrome://pocket/skin/menuPanel.png);
    -moz-image-region: rect(0, 32px, 32px, 0);
  }
  #pocket-button[cui-areatype="menu-panel"][panel-multiview-anchor=true] {
    -moz-image-region: rect(32px, 32px, 64px, 0);
  }

Note that this uses the same url, but a different image region.  

3) nsImageBoxFrame has different layout behavior from nsImageFrame in various subtle ways (since it's a XUL box with some custom bits, not a CSS replaced element).

That's based on a quick skim.  There may be some more subtle bits.

> How could I go about adding 'src' as a property to XUL <image> tags without using the XBL

The simplest thing is probably to create a new WebIDL interface inheriting from XULElement and have nsXULElement::WrapNode check the tagname and create an instance of the relevant interface.  That's what effectively ended up happening, except now the bindings will be creating that prototype object, instead of XBL doing it.

Then you put an "src" attribute on that interface and implement on nsXULElement.
Flags: needinfo?(bzbarsky)
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
(Reporter)

Comment 3

a year ago
Here's a push that just removes the XBL code: https://hg.mozilla.org/try/rev/e27416b553e3fc3d414ebe874dbbe241b9ae43c7.  It doesn't include the suggestion in Comment 2, so as expected we get some test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b332614577408f60cc37beaf4aea8e4e57226e
(Assignee)

Updated

a year ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
Here are a few results from this investigation. The nsIDOMXULImageElement interface is a red herring, it's not really used anywhere and the binding works just as well without it.

With regard to the "src" property, we could create a derived interface to implement it in C++, but it may be easier to just add the property to the base XULElement interface together with everything else that is already there.

The remaining item is the "role" attribute. The accessibility code walks the XBL bindings and finds the innermost binding with a "role" attribute. I've replaced this with a hard-coded check for the element's tag name, that must run before the walking occurs, and it seems to do the job. I also noticed some special-casing for toolbar buttons that may likely be removed.

As far as performance goes, removing this binding alone is not enough to move the needle in a significant way. The only confirmed improvement is in a test that is _not_ designed for XUL, so I guess it must be related to the test setup time or to side-effects in the browser UI.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2f2cf8e7359d4996962ebaf34defb7aa12939183&newProject=try&newRevision=e9233bb6cd77073829098c8bcf0d7b2df44735c4&framework=1&showOnlyImportant=1

There are a few regressions that I believe are mostly noise. To rule out the addition of one property to all XUL elements as the cause for the regressions, I've compared the new changeset with another one where I additionally removed the unused "statusText" property to balance it, and these regressions still show up between the two very similar changesets:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e9233bb6cd77073829098c8bcf0d7b2df44735c4&newProject=try&newRevision=4b16344328fa82e4e510c420da70fb7a07c3b63b&framework=1&showOnlyImportant=1

We could do more runs, but I'm not sure it's worth the infrastructure load at this stage.

So the investigation is done, but while this patch could land on its own, there are other directions:
1. See if there is a way to use this alternative method of looking up the role for more bindings.
2. See if there are other bindings for which we could move attribute setters/getters to C++.
3. Tackle some more common bindings and see if the performance gains become significant.
4. While here, remove some unused stuff from the XULElement interface as an incremental improvement.

Brian, thoughts?
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 8

a year ago
(In reply to :Paolo Amadini from comment #7)
> Here are a few results from this investigation. The nsIDOMXULImageElement
> interface is a red herring, it's not really used anywhere and the binding
> works just as well without it.
> 
> With regard to the "src" property, we could create a derived interface to
> implement it in C++, but it may be easier to just add the property to the
> base XULElement interface together with everything else that is already
> there.
> 
> The remaining item is the "role" attribute. The accessibility code walks the
> XBL bindings and finds the innermost binding with a "role" attribute. I've
> replaced this with a hard-coded check for the element's tag name, that must
> run before the walking occurs, and it seems to do the job. I also noticed
> some special-casing for toolbar buttons that may likely be removed.

Yura, does this approach seem reasonable? For image in particular, checking on tag name seems fine, but for future bindings we may want to still be able to specify via the `role` attribute. It looks like your approach here wouldn't preclude that.
Flags: needinfo?(yzenevich)
(Reporter)

Comment 9

a year ago
(In reply to :Paolo Amadini from comment #7)
> 2. See if there are other bindings for which we could move attribute
> setters/getters to C++.

Scanning https://bgrins.github.io/xbl-analysis/tree/#property I see "deck" and "iframe" are also bindings that only define properties and aren't extended, so they may be good next candidates if we decide this is the right approach.
(Reporter)

Comment 10

a year ago
(In reply to :Paolo Amadini from comment #7)
> Here are a few results from this investigation. The nsIDOMXULImageElement
> interface is a red herring, it's not really used anywhere and the binding
> works just as well without it.

That's interesting.

> With regard to the "src" property, we could create a derived interface to
> implement it in C++, but it may be easier to just add the property to the
> base XULElement interface together with everything else that is already
> there.

I'd like to see what Boris thinks about the approach, since he had the WebIDL suggestion originally.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 11

a year ago
(In reply to :Paolo Amadini from comment #7)
> 4. While here, remove some unused stuff from the XULElement interface as an
> incremental improvement.

Did you find anything in particular that could be removed? If so, we should spin it off into another bug regardless of what we decide to do here.
> I'd like to see what Boris thinks about the approach

It basically depends on the semantics we want for XULElement.  There's no real problem with sticking reflection properties like this directly on there, apart from the "it may not make sense for all instances" issue...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 13

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to :Paolo Amadini from comment #7)
> > 4. While here, remove some unused stuff from the XULElement interface as an
> > incremental improvement.
> 
> Did you find anything in particular that could be removed? If so, we should
> spin it off into another bug regardless of what we decide to do here.

Well, statusText is the obvious one, but there might be more. If there are few consumers of the property we may just convert them to a plain setAttribute call.
(Reporter)

Comment 14

a year ago
(In reply to :Paolo Amadini from comment #7)
> Here are a few results from this investigation. The nsIDOMXULImageElement
> interface is a red herring, it's not really used anywhere and the binding
> works just as well without it.
> 
> With regard to the "src" property, we could create a derived interface to
> implement it in C++, but it may be easier to just add the property to the
> base XULElement interface together with everything else that is already
> there.
> 
> The remaining item is the "role" attribute. The accessibility code walks the
> XBL bindings and finds the innermost binding with a "role" attribute. I've
> replaced this with a hard-coded check for the element's tag name, that must
> run before the walking occurs, and it seems to do the job. I also noticed
> some special-casing for toolbar buttons that may likely be removed.
> 
> As far as performance goes, removing this binding alone is not enough to
> move the needle in a significant way. The only confirmed improvement is in a
> test that is _not_ designed for XUL, so I guess it must be related to the
> test setup time or to side-effects in the browser UI.
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=2f2cf8e7359d4996962ebaf34defb7aa
> 12939183&newProject=try&newRevision=e9233bb6cd77073829098c8bcf0d7b2df44735c4&
> framework=1&showOnlyImportant=1
> 
> There are a few regressions that I believe are mostly noise. 

I've seen a lot of noise in those three tests on a bunch of talos pushes lately, so I'm not too concerned about them. FWIW if I uncheck 'important' (>2%) and instead check 'hide uncertain' I do see a number of high certainty ~1% improvements, but it's hard to say if that's just noise as well (https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2f2cf8e7359d4996962ebaf34defb7aa12939183&newProject=try&newRevision=e9233bb6cd77073829098c8bcf0d7b2df44735c4&framework=1&showOnlyConfident=1).

> 3. Tackle some more common bindings and see if the performance gains become
> significant.

Even if perf stays the same with this change, it's still worth doing. <image> accounts for ~25% of the total XBL instances created in a mochitest-browser run, second only behind <label>.
Flags: needinfo?(bgrinstead)
Posted patch 1403231 a11ySplinter Review
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to :Paolo Amadini from comment #7)
> > Here are a few results from this investigation. The nsIDOMXULImageElement
> > interface is a red herring, it's not really used anywhere and the binding
> > works just as well without it.
> > 
> > With regard to the "src" property, we could create a derived interface to
> > implement it in C++, but it may be easier to just add the property to the
> > base XULElement interface together with everything else that is already
> > there.
> > 
> > The remaining item is the "role" attribute. The accessibility code walks the
> > XBL bindings and finds the innermost binding with a "role" attribute. I've
> > replaced this with a hard-coded check for the element's tag name, that must
> > run before the walking occurs, and it seems to do the job. I also noticed
> > some special-casing for toolbar buttons that may likely be removed.
> 
> Yura, does this approach seem reasonable? For image in particular, checking
> on tag name seems fine, but for future bindings we may want to still be able
> to specify via the `role` attribute. It looks like your approach here
> wouldn't preclude that.

It seems reasonable. I talked to Alex (module owner) about it too and to avoid "hard-coded" check in favour of some declarativeness, I'm attaching a patch that cleans it up a little bit and allows to add future mappings in a more simple way. Your tests seem to pass with my patch too.
Flags: needinfo?(yzenevich)
Also, I believe we do fall back to checking role attribute so overriding with it should be possible.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
Thank you Yura! I've integrated the patch series with your patch.

New tryserver builds will have to wait until the tree is reopened.

Comment 21

a year ago
mozreview-review
Comment on attachment 8923825 [details]
Bug 1403231 - Create accessibles for the XUL "image" element using the tag name instead of the XBL role.

https://reviewboard.mozilla.org/r/194982/#review200004


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: accessible/base/nsAccessibilityService.cpp:1323
(Diff revision 1)
>  
>    for (uint32_t i = 0; i < ArrayLength(sMarkupMapList); i++)
>      mMarkupMaps.Put(*sMarkupMapList[i].tag, &sMarkupMapList[i]);
>  
> +#ifdef MOZ_XUL
> +  for (uint32_t i = 0; i < ArrayLength(sXULMapList); i++)

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
(Assignee)

Updated

a year ago
Blocks: 1413201
(Assignee)

Comment 22

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #9)
> I see "deck" and "iframe" are also bindings that only define properties and aren't extended

These are rarely instantiated and the properties aren't simple attribute getters and setters, this makes me think that we may want to follow a different approach, like custom elements.
(Reporter)

Comment 24

a year ago
(In reply to :Paolo Amadini from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > I see "deck" and "iframe" are also bindings that only define properties and aren't extended
> 
> These are rarely instantiated and the properties aren't simple attribute
> getters and setters, this makes me think that we may want to follow a
> different approach, like custom elements.

Looking at deck, I agree. I think iframe needs more investigation as far as how it is working now. For instance I'm not sure where the "src" property is implemented for it. The only properties defined in XBL (all readonly) are "docShell" "contentWindow" "webNavigation" and "contentDocument".
> For instance I'm not sure where the "src" property is implemented for it.

It's not implemented for <xul:iframe> that I know of.

Comment 26

a year ago
mozreview-review
Comment on attachment 8923825 [details]
Bug 1403231 - Create accessibles for the XUL "image" element using the tag name instead of the XBL role.

https://reviewboard.mozilla.org/r/194982/#review200086

::: accessible/base/nsAccessibilityService.h:322
(Diff revision 1)
>     */
>    static uint32_t gConsumers;
>  
>    nsDataHashtable<nsPtrHashKey<const nsAtom>, const mozilla::a11y::MarkupMapInfo*> mMarkupMaps;
> +#ifdef MOZ_XUL
> +  nsDataHashtable<nsPtrHashKey<const nsAtom>, const mozilla::a11y::XULMapInfo*> mXULMaps;

I like short XULMaps name, but it puts us out of sync with mMarkupMaps, so we probably should have a bug to rename mMarkupMaps into mHTMLMaps. I could live with XULMarkupMap though, if you prefer it. Just it'd be nice to sync those namings.

Also, XULMap name is probably more correct than XULMaps, since it's all about tag names mapping into functions, which means we deal with a single map.

::: accessible/base/nsAccessibilityService.cpp:256
(Diff revision 1)
>  }
>  
> +#ifdef MOZ_XUL
> +static Accessible*
> +New_MaybeImageOrToolbarButtonAccessible(nsIContent* aContent,
> +                                        Accessible* aContext)

It'd be nice to replace functions living in a separate file on lambdas, it could look like:

MARKUPMAP(
  image,
  [](nsIContent* node, Accessible* context) {
    if (node->HasAttr(kNameSpaceID_None, nsGkAtoms::onclick)) {
      return new XULToolbarButtonAccessible(node, context->Document());
    }
    return node->HasAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext)) ?
    new ImageAccessibleWrap(node, context->Document()) : nullptr;
)

You'd need to replace NewAccessible type on std::function for this and that should do a trick.

::: accessible/base/nsAccessibilityService.cpp:293
(Diff revision 1)
>  };
>  
> +#ifdef MOZ_XUL
> +#define XULMAP(atom, new_func) \
> +  { &nsGkAtoms::atom, new_func },
> +

there's no point to have XULMap macros, since it matches to MarkupMap macros, which can be reused, right?
Attachment #8923825 - Flags: review?(surkov.alexander) → review+

Comment 27

a year ago
mozreview-review
Comment on attachment 8923302 [details]
Bug 1403231 - Add accessibility tests for the XUL "image" element.

https://reviewboard.mozilla.org/r/194486/#review200108

It'd be nicer to create elm/test_XULSpec.html file or whathever name is more appropriate for XUL, similar to test_HTMLSpec.html and test_MathMLSpec.html files - these files provide a basic testing of elements of a certain namespace. I'm ok with the taken approach though, since it also works. Note, image@onclick case is missing.
Attachment #8923302 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 28

a year ago
mozreview-review
Comment on attachment 8923303 [details]
Bug 1403231 - Remove the "image" XBL binding.

https://reviewboard.mozilla.org/r/194488/#review200128

This looks good to me for the frontend changes - I can't find anywhere else that references the deleted image binding
Attachment #8923303 - Flags: review?(bgrinstead) → review+

Comment 29

a year ago
mozreview-review
Comment on attachment 8923303 [details]
Bug 1403231 - Remove the "image" XBL binding.

https://reviewboard.mozilla.org/r/194488/#review200242

r=me
Attachment #8923303 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 30

a year ago
(In reply to alexander :surkov from comment #27)
> Note, image@onclick case is missing.

Hm, I thought it was a legacy case, but apparently we still use it in the navigation toolbar:

https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/browser/base/content/urlbarBindings.xml#43-47
(Assignee)

Comment 31

a year ago
Actually, the new Talos builds show less improvement than before:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1ddb7a66728054fed6ee87d3dd1288f29f1154ee&newProject=try&newRevision=5050177333353753bd0357f3970845fcb845d150&framework=1&showOnlyConfident=1

I suspect that using a hashtable at this early stage may be what is slowing down the builds. If this is confirmed, probably the simple comparison is better for now, and going forward it may be even better to use a serial lookup similar to what CreateAccessibleByType already does, prioritized by tag frequency. In fact, hashtables with less than 10-12 elements are expected to be slower than array lookups.
(Reporter)

Comment 32

a year ago
(In reply to :Paolo Amadini from comment #31)
> Actually, the new Talos builds show less improvement than before:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=1ddb7a66728054fed6ee87d3dd1288f2
> 9f1154ee&newProject=try&newRevision=5050177333353753bd0357f3970845fcb845d150&
> framework=1&showOnlyConfident=1
> 
> I suspect that using a hashtable at this early stage may be what is slowing
> down the builds. If this is confirmed, probably the simple comparison is
> better for now, and going forward it may be even better to use a serial
> lookup similar to what CreateAccessibleByType already does, prioritized by
> tag frequency. In fact, hashtables with less than 10-12 elements are
> expected to be slower than array lookups.

FWIW I don't think there are any other elements that will follow this exact pattern of migration that we are using here. Although depending on how we want to handle roles with Custom Elements we may want to reuse this infrastructure. I can think of a few ways:

1) Use this infrastructure, and coordinate landing a customElement.define with a change to XULMap.h to match the role that used to be assigned due to the role attribute in XBL (may also require adding a new nsGkAtom depending on if the tag name is already there)
2) Somehow store the default role in the CustomElementRegistry (maybe as a chrome-only property in `options` as https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define#Parameters)
3) Set the 'role' attribute inside of each connectedCallback for Custom Elements (seems inefficient though)

I'd prefer (2) for the frontend since the role could be specified right next to the definition, and could be automatically converted from a XBL binding. But I don't know how much work it would be to implement on the accessibility side and (1) seems fine as well since Custom Elements are defined by a tag name.
(Assignee)

Comment 34

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #32)
> I can think of a few ways:

I don't know which of these options could be better with regard to performance, but I suspect that the fastest would be the equivalent of constructing the accessible object, or specifying its factory, together with any construction necessary for the custom element itself. It seems solutions 2 and 3 are closer to this ideal, but it's difficult to tell without testing actual code.
(In reply to :Paolo Amadini from comment #31)
> Actually, the new Talos builds show less improvement than before:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=1ddb7a66728054fed6ee87d3dd1288f2
> 9f1154ee&newProject=try&newRevision=5050177333353753bd0357f3970845fcb845d150&
> framework=1&showOnlyConfident=1
> 
> I suspect that using a hashtable at this early stage may be what is slowing
> down the builds. If this is confirmed, probably the simple comparison is
> better for now, and going forward it may be even better to use a serial
> lookup similar to what CreateAccessibleByType already does, prioritized by
> tag frequency. In fact, hashtables with less than 10-12 elements are
> expected to be slower than array lookups.

CreateAccessibleByType contains about 40 XUL elements to run through, so we might benefit from a map. Having said that if a few elements like image and label make 80% or so of all elements, then it should be reasonable to have prioritized ifs.
(Reporter)

Comment 36

a year ago
(In reply to alexander :surkov from comment #35)
> CreateAccessibleByType contains about 40 XUL elements to run through, so we
> might benefit from a map. Having said that if a few elements like image and
> label make 80% or so of all elements, then it should be reasonable to have
> prioritized ifs.

The best data we have so far is that image and label make up around 50% of the elements (this is gathered during a mochitest-browser run). Sidenote: I'm working to make this data easier to gather in Bug 1376546 and a graph of it can be seen here https://bgrins.github.io/xbl-analysis/graph/#bindings-instances.
(Reporter)

Updated

a year ago
Whiteboard: [xbl-special-cases]
(Assignee)

Updated

a year ago
Blocks: 1414230
(Assignee)

Comment 37

a year ago
I've run a number of Talos tests comparing the two approaches, and the latest runs show the simple approach as generally slower than the hashtable, which is the opposite of the earlier runs. This leads me to think that the performance impact is not measurable overall for the test cases on Talos, which are the ones we care about. So I'll go ahead and land the hashtable version. I've done the rename, and I've filed bug 1414230 for the rest of the clean up suggested by the review.

Comment 38

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/469018312d09
Add accessibility tests for the XUL "image" element. r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/58101c3badd5
Create accessibles for the XUL "image" element using the tag name instead of the XBL role. r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/baeb2e4f4c1c
Remove the "image" XBL binding. r=bz,bgrins
Comment hidden (obsolete)
Comment hidden (obsolete)
https://hg.mozilla.org/mozilla-central/rev/469018312d09
https://hg.mozilla.org/mozilla-central/rev/58101c3badd5
https://hg.mozilla.org/mozilla-central/rev/baeb2e4f4c1c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1414522
Summary: Prototype removal of "image" XBL binding → Remove "image" XBL binding
(Reporter)

Updated

a year ago
Blocks: 1416363
(Reporter)

Updated

a year ago
See Also: → bug 1416493
(Reporter)

Updated

a year ago
Blocks: 1428930

Updated

a year ago
See Also: → bug 1437638
You need to log in before you can comment on or make changes to this bug.