Closed Bug 1279665 Opened 8 years ago Closed 8 years ago

No longer creating accessibles for the one-off search buttons for the toolbar's search field

Categories

(Core :: Disability Access APIs, defect, P1)

Product:
Core
Component:
Disability Access APIs
Version:
48 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed
firefox50 + verified

People

(Reporter: MarcoZ, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(1 file)

ignore XUL elements for early aria-owns detection
1.31 KB, patch
yzen
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review 
STR:
1. With NVDA running, start Firefox.
2. Pres CTRL+K and add some text.
3. Tab.

Expected: NVDA should say "Search for <your entered text> using "Google", or whichever search engine name is focused.

actual: Silence.

4. Examine the accessible tree with a tool of your choosing.

Result: The one-off search buttons are nowhere to be found. There are a few unknown accessibles in the vicinity, but no search buttons any more.

Gijs, did the markup for the search buttons for the search box (not the about:newtab page) change recently? Or is this solely the Core Accessibility code's fault? ;-)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Marco Zehe (:MarcoZ) from comment #0)
> Gijs, did the markup for the search buttons for the search box (not the
> about:newtab page) change recently? Or is this solely the Core Accessibility
> code's fault? ;-)

I don't know. Florian?

(a regression range might be helpful either way - it used to work, so Something changed - no matter what exactly that was, it would be useful to find out...). I take it from the flags this works correctly with 48 (current beta)?
Flags: needinfo?(mzehe)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
(In reply to :Gijs Kruitbosch from comment #1)
> (a regression range might be helpful either way - it used to work, so
> Something changed - no matter what exactly that was, it would be useful to
> find out...). I take it from the flags this works correctly with 48 (current
> beta)?
Unfortunately it's broken there, too. When I filed the bug, I just hadn't tested on beta yet. I don't use these often, so this just completely slipped by me. And we don't seem to cover this particular markup in our mochitest suite, so this got unnoticed. I suspect it's more in the accessibility code than the one-off search buttons, but need to confirm that. There is a prime suspect in 48 that caused a few other regressions, bug 1261425, but I haven't confirmed that with a regression window yet. Preparing for MozLondon, and it was the weekend. :-)
status-firefox48: --- → affected
Flags: needinfo?(mzehe)
[Tracking Requested - why for this release]:
Serious functional regression in a11y support for main browser UI.
tracking-firefox48: --- → ?
I talked to Gijs at dinner last night, and there are no changes to the search buttons that could account for this regression. Alex, please take over and investigate why we're no longer creating accessibles for these. The regression is in the many bigger changes that happened in 48, prime suspect is bug 1261425, but not sure, and I don't know if I'll have a chance to find a regression range this week. Might just be faster you check this over via debugging. Thanks!
Flags: needinfo?(florian) → needinfo?(surkov.alexander)
I'm not sure about bug 1261425, because it was about eventing, and thus I wouldn't expect tree changes because of it.

I can see two different UIs. Default profile on Firefox 47 loads a search page into the current tab on cmd+K (no search field in the browser's toolbar), while Nightly has a classic view in debug profile. Assuming the issue was related to the classical view, I inspected the toolbar tree.

There's no search button, but there's a search graphic accessible. It's just an image inside a hbox. In contrast to other toolbar item's text fields, they use either toolbarbuttons or role="button" on hbox containing an image. I'd say it's likely UI issue.
Flags: needinfo?(surkov.alexander)
Alex, it's the one when you press Ctrl+K on Windows, Cmd+K on Mac, and type something into that field. I am not talking about about:newtab's search UI. And I've confirmed that this is broken in 48 Beta, too.
Alex can you confirm STR with comment 6 works for you?
Flags: needinfo?(surkov.alexander)
(In reply to David Bolter [:davidb] from comment #7)
> Alex can you confirm STR with comment 6 works for you?

see comment #5
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #8)
> (In reply to David Bolter [:davidb] from comment #7)
> > Alex can you confirm STR with comment 6 works for you?
> 
> see comment #5

OK so reading this whole bug over seems to point to /accessible being the likely module that changed. Assigning to that module's owner :)
Assignee: nobody → surkov.alexander
(In reply to David Bolter [:davidb] from comment #9)

> OK so reading this whole bug over seems to point to /accessible being the
> likely module that changed.

what does it make you think so?
(In reply to alexander :surkov from comment #10)
> (In reply to David Bolter [:davidb] from comment #9)
> 
> > OK so reading this whole bug over seems to point to /accessible being the
> > likely module that changed.
> 
> what does it make you think so?

Reading over the entire bug but especially comment 4. If you think the search box drown-down markup has changed and is causing this bug then you should probably NI Gijs or Florian. Or maybe catch them in IRC?
Maybe this 'regression' has been around a long time?
What I mean by comment 12... I had an old FF version 44 release lying around and I saw the (command+k) search field drop down which appeared visually to be the same as nightly (to me).
Gijs is that search drop-down UI done/updated via go-faster?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to David Bolter [:davidb] from comment #11)

> Reading over the entire bug but especially comment 4. If you think the
> search box drown-down markup has changed and is causing this bug then you
> should probably NI Gijs or Florian. Or maybe catch them in IRC?

I don't affirm that was UI change, but I don't exclude that too. Anyway, there are some UI concerns I outlined in comment #5 that'd be useful to get answered.
(In reply to David Bolter [:davidb] from comment #14)
> Gijs is that search drop-down UI done/updated via go-faster?

It is not. A real regression window will likely help here. I'll try to look tomorrow if nobody else does, I guess. :-\
mozregression says: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=be4f368296b7ba048ca030ad5e5e447ad0456cfe&tochange=3e2a9133cb5cb70cf5458db451f451de1223ab10

This is a regression from bug 1261408. :surkov, can you take a look? I don't know what that patch is supposed to do, but given the fixes we made in bug 1107695 to make these buttons work, it seems this breaks for aria-activedescendant mixed with anon content, or something.
Blocks: 1261408
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch from comment #17)

> This is a regression from bug 1261408. :surkov, can you take a look? I don't
> know what that patch is supposed to do, but given the fixes we made in bug
> 1107695 to make these buttons work, it seems this breaks for
> aria-activedescendant mixed with anon content, or something.

Thanks for finding the regression. That bug may change how we build looping ARIA owns trees, not sure if that is the issue here.

Anyway, I don't recall any aria-owns in that UI. As I wrote in comment #5 I saw an image used as a button that has nothing in markup that would make it a button. Could you please refer me to inaccessible markup?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> 
> > This is a regression from bug 1261408. :surkov, can you take a look? I don't
> > know what that patch is supposed to do, but given the fixes we made in bug
> > 1107695 to make these buttons work, it seems this breaks for
> > aria-activedescendant mixed with anon content, or something.
> 
> Thanks for finding the regression. That bug may change how we build looping
> ARIA owns trees, not sure if that is the issue here.
> 
> Anyway, I don't recall any aria-owns in that UI. As I wrote in comment #5 I
> saw an image used as a button that has nothing in markup that would make it
> a button. Could you please refer me to inaccessible markup?

https://dxr.mozilla.org/mozilla-central/search?q=aria-owns+path%3Abrowser%2F&redirect=false

Based on that, this might also break the search suggestions opt-in in the awesomebar.
(In reply to alexander :surkov from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> 
> > This is a regression from bug 1261408. :surkov, can you take a look? I don't
> > know what that patch is supposed to do, but given the fixes we made in bug
> > 1107695 to make these buttons work, it seems this breaks for
> > aria-activedescendant mixed with anon content, or something.
> 
> Thanks for finding the regression. That bug may change how we build looping
> ARIA owns trees, not sure if that is the issue here.
> 
> Anyway, I don't recall any aria-owns in that UI. As I wrote in comment #5 I
> saw an image used as a button that has nothing in markup that would make it
> a button. Could you please refer me to inaccessible markup?

It's not actually clear what you mean here, thinking about it... the buttons are generated dynamically, and when focused we set aria-activedescendant on them: https://dxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#784

Note also that, as mentioned in comment 6, we're talking about the search field in the main browser toolbar, not about:newtab. If the popup disappearing is interfering with your debugging, you can use the browser toolbox ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox ) to make the popup stay open (click the button with four little squares on it on the top right).
apparently I was looking at another search button, which is a child of search autocomplete. Just to be clear the accessible tree, I was looking at, is:
autocomplete
  graphic ('magnifying glass' button, 'search' name)
  entry (for textbox)
  combobox_list (for popup)
  pushbutton search ('arrow' button, 'serach' name)

that probably yet one more bug.

aria-activedescendant is likely unrelated, since marco reported that those 'search engine' buttons are not accessible (if i'm looking at right UI piece again).

I see that combobox_list (xul:menupoup) is empty and all buttons are inserted under the document tree. Could you give me a code snippet for better understanding how that xul:menupopup is connected with those buttons?
(In reply to alexander :surkov from comment #21)
> apparently I was looking at another search button, which is a child of
> search autocomplete. Just to be clear the accessible tree, I was looking at,
> is:
> autocomplete
>   graphic ('magnifying glass' button, 'search' name)
>   entry (for textbox)
>   combobox_list (for popup)
>   pushbutton search ('arrow' button, 'serach' name)
> 
> that probably yet one more bug.

"that" is what, in this context?

> aria-activedescendant is likely unrelated, since marco reported that those
> 'search engine' buttons are not accessible (if i'm looking at right UI piece
> again).

I don't understand. They were accessible before your patch. Before that patch, they were only accessible after the changes from bug 1107695. They were not accessible before that, because without the aria-owns and aria-activedescendant markup that we added there, the buttons were not in the a11y tree, presumably because the a11y code did not pick them up. Focus remains in the textbox at all times, so that typing 'just works', while we somehow have to tell the user that the 'fake' focus moves from button to button when tabbing through the popup, which is what the activedescendant markup is used for. Given that the same version of NVDA works on old builds and stopped announcing things after the cset from bug 1261408, I can only assume that somehow the markup no longer has the desired effect. Please see the other bug for the discussion that led to the markup we use here.

> I see that combobox_list (xul:menupoup) is empty and all buttons are
> inserted under the document tree.

I don't really understand this, either. What is the menupopup? Are you sure it's a <menupopup> ? I don't think the autocomplete result popup is a menupopup at all, it's a <panel>. What are "all buttons", and where is "under the document tree"?

> Could you give me a code snippet for
> better understanding how that xul:menupopup is connected with those buttons?

Again, not sure what buttons and menupopup we're talking about. At a guess, if "buttons" is the one-off search buttons, this is where they get inserted:

https://dxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#1328-1349

They get inserted in a XUL <description> that's in the anon content of the <panel>'s binding, with anonid="search-panel-one-offs". Does that help?
Flags: needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch from comment #22)
> They
> were not accessible before that, because without the aria-owns and
> aria-activedescendant markup that we added there, the buttons were not in
> the a11y tree, presumably because the a11y code did not pick them up.

Actually, I should be specific: I don't know if they were in the a11y tree or not (try a build from 2015-01-01 to see) but they were certainly not accessible, ie focus changes between the buttons did not get announced / picked up by the screen reader.
(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to alexander :surkov from comment #21)
> > apparently I was looking at another search button, which is a child of
> > search autocomplete. Just to be clear the accessible tree, I was looking at,
> > is:
> > autocomplete
> >   graphic ('magnifying glass' button, 'search' name)
> >   entry (for textbox)
> >   combobox_list (for popup)
> >   pushbutton search ('arrow' button, 'serach' name)
> > 
> > that probably yet one more bug.
> 
> "that" is what, in this context?

'magnifying loop with green plus sign' image inside the searchbox is a button semantically, but it is exposed as a graphic role (image).

> > aria-activedescendant is likely unrelated, since marco reported that those
> > 'search engine' buttons are not accessible (if i'm looking at right UI piece
> > again).
> 
> I don't understand. They were accessible before your patch.

I was referred to aria-activedescendant which is focus related thing, it doesn't affect on the accessible tree, here we deal with accessible tree problem I think.

>Before that
> patch, they were only accessible after the changes from bug 1107695. They
> were not accessible before that, because without the aria-owns and
> aria-activedescendant markup that we added there, the buttons were not in
> the a11y tree, presumably because the a11y code did not pick them up. Focus
> remains in the textbox at all times, so that typing 'just works', while we
> somehow have to tell the user that the 'fake' focus moves from button to
> button when tabbing through the popup, which is what the activedescendant
> markup is used for. Given that the same version of NVDA works on old builds
> and stopped announcing things after the cset from bug 1261408, I can only
> assume that somehow the markup no longer has the desired effect. 

yes, aria-owns likely caused a problem here

> > I see that combobox_list (xul:menupoup) is empty and all buttons are
> > inserted under the document tree.
> 
> I don't really understand this, either. What is the menupopup? Are you sure
> it's a <menupopup> ? I don't think the autocomplete result popup is a
> menupopup at all, it's a <panel>. What are "all buttons", and where is
> "under the document tree"?

the accessible tree is:

autocomplete xul:textbox@anonid="searchbar-textbox"
  graphic xul:image@anonid="searchbar-search-button"
  entry xul:textbox@anonid="input"
  combobox_list xul:menupopup@anonid="input-box-contextmenu"
Track this as this is a regression in a11y.
tracking-firefox48: ? → +
tracking-firefox49: --- → +
tracking-firefox50: --- → +
(In reply to alexander :surkov from comment #24)
> (In reply to :Gijs Kruitbosch from comment #22)
> > (In reply to alexander :surkov from comment #21)
> > > apparently I was looking at another search button, which is a child of
> > > search autocomplete. Just to be clear the accessible tree, I was looking at,
> > > is:
> > > autocomplete
> > >   graphic ('magnifying glass' button, 'search' name)
> > >   entry (for textbox)
> > >   combobox_list (for popup)
> > >   pushbutton search ('arrow' button, 'serach' name)
> > > 
> > > that probably yet one more bug.
> > 
> > "that" is what, in this context?
> 
> 'magnifying loop with green plus sign' image inside the searchbox is a
> button semantically, but it is exposed as a graphic role (image).

We should have a discussion about what to do for this in a separate bug. Right now it's not important.

> 
> > > aria-activedescendant is likely unrelated, since marco reported that those
> > > 'search engine' buttons are not accessible (if i'm looking at right UI piece
> > > again).
> > 
> > I don't understand. They were accessible before your patch.
> 
> I was referred to aria-activedescendant which is focus related thing, it
> doesn't affect on the accessible tree, here we deal with accessible tree
> problem I think.
> 
> >Before that
> > patch, they were only accessible after the changes from bug 1107695. They
> > were not accessible before that, because without the aria-owns and
> > aria-activedescendant markup that we added there, the buttons were not in
> > the a11y tree, presumably because the a11y code did not pick them up. Focus
> > remains in the textbox at all times, so that typing 'just works', while we
> > somehow have to tell the user that the 'fake' focus moves from button to
> > button when tabbing through the popup, which is what the activedescendant
> > markup is used for. Given that the same version of NVDA works on old builds
> > and stopped announcing things after the cset from bug 1261408, I can only
> > assume that somehow the markup no longer has the desired effect. 
> 
> yes, aria-owns likely caused a problem here
> 
> > > I see that combobox_list (xul:menupoup) is empty and all buttons are
> > > inserted under the document tree.
> > 
> > I don't really understand this, either. What is the menupopup? Are you sure
> > it's a <menupopup> ? I don't think the autocomplete result popup is a
> > menupopup at all, it's a <panel>. What are "all buttons", and where is
> > "under the document tree"?
> 
> the accessible tree is:
> 
> autocomplete xul:textbox@anonid="searchbar-textbox"
>   graphic xul:image@anonid="searchbar-search-button"
>   entry xul:textbox@anonid="input"
>   combobox_list xul:menupopup@anonid="input-box-contextmenu"

Right, so none of this is the thing you want. Why is the aria-owned panel missing?
(In reply to :Gijs Kruitbosch from comment #26)

> > the accessible tree is:
> > 
> > autocomplete xul:textbox@anonid="searchbar-textbox"
> >   graphic xul:image@anonid="searchbar-search-button"
> >   entry xul:textbox@anonid="input"
> >   combobox_list xul:menupopup@anonid="input-box-contextmenu"
> 
> Right, so none of this is the thing you want. Why is the aria-owned panel
> missing?

who is owning that panel? what manages aria-owns attribute? can you think of a code snippet to demo the problem?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> 
> > > the accessible tree is:
> > > 
> > > autocomplete xul:textbox@anonid="searchbar-textbox"
> > >   graphic xul:image@anonid="searchbar-search-button"
> > >   entry xul:textbox@anonid="input"
> > >   combobox_list xul:menupopup@anonid="input-box-contextmenu"
> > 
> > Right, so none of this is the thing you want. Why is the aria-owned panel
> > missing?
> 
> who is owning that panel? what manages aria-owns attribute? can you think of
> a code snippet to demo the problem?

I don't understand. Can't you just reproduce the problem as it happens with the search bar? That is pretty much the simplest way to see this. I don't see the point in coming up with new code snippets when we have a regression window and a clear testcase already. To see the search bar owns this popup, in the browser console, you can just run:

document.getAnonymousElementByAttribute(document.getElementById("searchbar"), "anonid", "searchbar-textbox").getAttribute("aria-owns")

which produces "PopupSearchAutoComplete".
Flags: needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch from comment #28)

> I don't understand. Can't you just reproduce the problem as it happens with
> the search bar? That is pretty much the simplest way to see this. I don't
> see the point in coming up with new code snippets when we have a regression
> window and a clear testcase already.

the point is simplify the debugging, since tons of stuff triggered in a11y, when you debug the browser, and also it'd be good to put that into mochitest to prevent us from breaking in the future. Does it make sense?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #29)
> (In reply to :Gijs Kruitbosch from comment #28)
> 
> > I don't understand. Can't you just reproduce the problem as it happens with
> > the search bar? That is pretty much the simplest way to see this. I don't
> > see the point in coming up with new code snippets when we have a regression
> > window and a clear testcase already.
> 
> the point is simplify the debugging, since tons of stuff triggered in a11y,
> when you debug the browser, and also it'd be good to put that into mochitest
> to prevent us from breaking in the future. Does it make sense?

It makes sense you want a testcase for your ability to fix this or to avoid it regressing.

It doesn't make sense that you want me to write it for you. I found you a regression window, and I've explained what the component parts of the browser are doing here, but I didn't break this, I don't know the a11y code, and I don't have the time to go dig into this pile of stuff only to make a thing that reproduces the same bug that's reproducible from an end-user perspective already right there in the browser. It'll involve XUL and XBL and be a pain to come up with and then test. A browser test that just uses the existing UI would be easier but even that would require reading up on the a11y APIs and would apparently not help you debug. So I think the best way forward here is you writing whatever you need to get this fixed.
this bug is stretched far from the a11y API boundaries, and it'd be beneficial to have a help in localizing a problem from someone who's more experienced in the browser UI internals than me, since personally I didn't touch this area for years from now. It shouldn't require a deep knowledge of a11y APIs, except some of ARIA of course. I'm not pushing on you, and if you don't have time for this and don't know anyone who can be interested to help with it, then it's ok too. Just if I had a small test case, then I would address a problem much faster. That's pretty much it.
(In reply to :Gijs Kruitbosch from comment #28)

> document.getAnonymousElementByAttribute(document.getElementById("searchbar"),
> "anonid", "searchbar-textbox").getAttribute("aria-owns")
> 
> which produces "PopupSearchAutoComplete".

the problem here is "searchbar-textbox" is in anonnymous subtree so it cannot refer to explicit content by ID, in other words aria-owns was used incorrectly in XBL case in this particular case. That means if you remove aria-owns from "searchbar-textbox" then it should fix the problem, because it will make all things looking exactly same as they were before bug 1261408.
Marco, can you still reproduce this on 48 beta 5? Because I can't.(In reply to alexander :surkov from comment #32)
> (In reply to :Gijs Kruitbosch from comment #28)
> 
> > document.getAnonymousElementByAttribute(document.getElementById("searchbar"),
> > "anonid", "searchbar-textbox").getAttribute("aria-owns")
> > 
> > which produces "PopupSearchAutoComplete".
> 
> the problem here is "searchbar-textbox" is in anonnymous subtree so it
> cannot refer to explicit content by ID, in other words aria-owns was used
> incorrectly in XBL case in this particular case. That means if you remove
> aria-owns from "searchbar-textbox" then it should fix the problem, because
> it will make all things looking exactly same as they were before bug 1261408.

Did you try this suggestion yourself? Removing this attribute on Nightly using the browser console makes NVDA read out all the one-off buttons as "unknown", instead of not reading anything. Adding the aria-owns markup to the searchbar itself, which isn't anon content, doesn't help either.

You've also not really explained why this used to work before bug 1261408 and why that bug would break it, so I don't know what else to suggest.
Flags: needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch from comment #33)
> Marco, can you still reproduce this on 48 beta 5? Because I can't.

I should have removed this before submitting. Something must somehow cause this to work in some circumstances, but after a browser restart it was back to being broken on 48 as well.
(In reply to :Gijs Kruitbosch from comment #33)
> Marco, can you still reproduce this on 48 beta 5? Because I can't.(In reply
> to alexander :surkov from comment #32)
> > (In reply to :Gijs Kruitbosch from comment #28)
> > 
> > > document.getAnonymousElementByAttribute(document.getElementById("searchbar"),
> > > "anonid", "searchbar-textbox").getAttribute("aria-owns")
> > > 
> > > which produces "PopupSearchAutoComplete".
> > 
> > the problem here is "searchbar-textbox" is in anonnymous subtree so it
> > cannot refer to explicit content by ID, in other words aria-owns was used
> > incorrectly in XBL case in this particular case. That means if you remove
> > aria-owns from "searchbar-textbox" then it should fix the problem, because
> > it will make all things looking exactly same as they were before bug 1261408.
> 
> Did you try this suggestion yourself?

nope, I don't have a windows build at hands

> Removing this attribute on Nightly
> using the browser console makes NVDA read out all the one-off buttons as
> "unknown", instead of not reading anything. Adding the aria-owns markup to
> the searchbar itself, which isn't anon content, doesn't help either.

hm, it might be something else. 'unknown' usually points at no accessible, I'm not sure if a panel element itself is accessible by default (aria-owns pointing to it would make it accessible), and whether it screws up aria-activedescendant things. I can do a backout try build to see if it makes a difference.

> You've also not really explained why this used to work before bug 1261408
> and why that bug would break it, so I don't know what else to suggest.

in that bug the behavior was changed this way: if we see that an element is referred by aria-owns attribute, then we delay its accessible creation until aria-owns is processed; but when aria-owns gets processed, we fail to find an element for the given ID, because autocomplete is in anonymous content, and thus we don't create an accessible. Before that patch we created an accessible for element from beginning, and then we were supposed to move it in the tree, when aria-owns is processed (I think we didn't do that, because aria-owns is in anonymous content). That made me think that aria-owns removal should turn the thing to their previous state.
Flags: needinfo?(surkov.alexander)
Priority: -- → P1
(In reply to alexander :surkov from comment #37)
> here's a try build that disables the introduced behavior for XUL -
> https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.
> com-0ad28cfe24f77cd2bfca543c2c1c3dfde7e5b62e/.
I can confirm that this try build fixes the problem. Did not try the build from comment #38, though, only this one.
(In reply to Marco Zehe (:MarcoZ) from comment #39)
> (In reply to alexander :surkov from comment #37)
> > here's a try build that disables the introduced behavior for XUL -
> > https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.
> > com-0ad28cfe24f77cd2bfca543c2c1c3dfde7e5b62e/.
> I can confirm that this try build fixes the problem.

so I'll file a patch for review then, to stop bleeding and process nicely the bad authoring, but it's worth to fix UI anyway.

> Did not try the build
> from comment #38, though, only this one.

nah, that was build from different bug, I just put a wrong commit message on that push, so it's good you ignored it :)
Comment on attachment 8768816 [details] [diff] [review]
ignore XUL elements for early aria-owns detection

Review of attachment 8768816 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok, one comment and test would've been great?

::: accessible/base/TreeWalker.cpp
@@ +294,5 @@
> +    // create an accessible object for it, when aria-owns is processed, which
> +    // may make the element subtree inaccessible. To avoid that let's create
> +    // an accessible object now, and later, if allowed, move it in the tree,
> +    // when aria-owns relation is processed.
> +    if (mDoc->RelocateARIAOwnedIfNeeded(aNode) && !aNode->IsXULElement()) {

Isn't there a way to determine if a node is in an anonymous context? IsXULElement seems to broad?
Attachment #8768816 - Flags: review?(yzenevich) → review+
(In reply to Yura Zenevich [:yzen] from comment #42)

> Looks ok, one comment and test would've been great?

it would be a nice addition, but the problem is about bad authoring and restricted to XBL/XUL world, i.e. to Mozilla internals, we probably won't ever run into it, but XUL/XBL mochitest would need some time.

> ::: accessible/base/TreeWalker.cpp
> @@ +294,5 @@
> > +    // create an accessible object for it, when aria-owns is processed, which
> > +    // may make the element subtree inaccessible. To avoid that let's create
> > +    // an accessible object now, and later, if allowed, move it in the tree,
> > +    // when aria-owns relation is processed.
> > +    if (mDoc->RelocateARIAOwnedIfNeeded(aNode) && !aNode->IsXULElement()) {
> 
> Isn't there a way to determine if a node is in an anonymous context?
> IsXULElement seems to broad?

that's probably ok, XUL has quite restricted usage. We could make precise checks to detect the case (basically copy of IDRefsIterator logic), but not sure if worth it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d78eccd3f37ca7e96c9755a058c6b60d3aa91276
Bug 1279665 - skip aria-owns deferred accessible creation for XUL elements, r=yzen
status-firefox47: --- → unaffected
Version: Trunk → 48 Branch
https://hg.mozilla.org/mozilla-central/rev/d78eccd3f37c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Hi Alexander, do you want to uplift this for 48/49 if this patch is not too risky?
Flags: needinfo?(surkov.alexander)
(In reply to Gerry Chang [:gchang] from comment #47)
> Hi Alexander, do you want to uplift this for 48/49 if this patch is not too
> risky?
Answering for Alex. Yes we do want this in 48 and 49, this patch merely reverts some behavior introduced in bug 1261408, which affects XUL, but not HTML and therefore has reduced impact, but fixes an important user-facing regression with the one-off search buttons in the search box at the top right.
Flags: needinfo?(surkov.alexander)
Comment on attachment 8768816 [details] [diff] [review]
ignore XUL elements for early aria-owns detection

Approval Request Comment
[Feature/regressing bug #]: bug 1261408
[User impact if declined]: Screen reader users will no longer hear the focused one-off button in the toolbar's search box.
[Describe test coverage new/current, TreeHerder]: Treeherder, manual verification, landed on Central.
[Risks and why]: Low risk, excludes XUL and XBL from an optimization in bug 1261408 and therefore only has internal impact.
[String/UUID change made/needed]: None.
Attachment #8768816 - Flags: approval-mozilla-beta?
Attachment #8768816 - Flags: approval-mozilla-aurora?
Landing of fix verified in 2016-07-10 Nightly build.
status-firefox50: fixed → verified
Comment on attachment 8768816 [details] [diff] [review]
ignore XUL elements for early aria-owns detection

Review of attachment 8768816 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression and is also verified. Take it in 48 beta 7 and aurora.
Attachment #8768816 - Flags: approval-mozilla-beta?
Attachment #8768816 - Flags: approval-mozilla-beta+
Attachment #8768816 - Flags: approval-mozilla-aurora?
Attachment #8768816 - Flags: approval-mozilla-aurora+
Marco, would you please file follow up bugs for UI issues (aria-owns one and commet #24)?
(In reply to alexander :surkov from comment #54)
> Marco, would you please file follow up bugs for UI issues (aria-owns one and
> commet #24)?
Filed bug 1286211 for the button, and bug 1286212 for the aria-owns usage issue.
(In reply to Marco Zehe (:MarcoZ) from comment #55)
> (In reply to alexander :surkov from comment #54)
> > Marco, would you please file follow up bugs for UI issues (aria-owns one and
> > commet #24)?
> Filed bug 1286211 for the button, and bug 1286212 for the aria-owns usage
> issue.

thank you
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: