Closed Bug 1403231 Opened 7 years ago Closed 7 years ago

Remove "image" XBL binding

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: bgrins, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [xbl-special-cases])

Attachments

(4 files)

Bug 1403231 - Add accessibility tests for the XUL "image" element.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1403231 - Remove the "image" XBL binding.
59 bytes, text/x-review-board-request
bgrins
: review+
bzbarsky
: review+
Details
1403231 a11y
8.12 KB, patch
Details | Diff | Splinter Review
Bug 1403231 - Create accessibles for the XUL "image" element using the tag name instead of the XBL role.
59 bytes, text/x-review-board-request
surkov
: review+
Details 
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.
Priority: -- → P5
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)
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: nobody → paolo.mozmail
Status: NEW → ASSIGNED
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)
(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)
(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.
(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)
(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)
(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.
(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)
Attached patch 1403231 a11y — — Splinter 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.
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 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]
Blocks: 1413201
(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.
(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 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 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+
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 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+
(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
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.
(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.
(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.
(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.
Whiteboard: [xbl-special-cases]
Blocks: 1414230
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.
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
Depends on: 1414522
Summary: Prototype removal of "image" XBL binding → Remove "image" XBL binding
Blocks: 1416363
See Also: → 1416493
Blocks: 1428930
See Also: → 1437638
Type: enhancement → task
See Also: → 1584641
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: