Closed Bug 893153 Opened 8 years ago Closed 8 years ago

[AccessFu] Navigation continues in hidden iframes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 1 obsolete file)

The iframe could be hidden with the current traversal rule, yet we still will move into items in it, since we don't check it for visibility.
Comment on attachment 774835 [details] [diff] [review]
Virtual cursor control refactor, fixes navigating in hidden frames.

This needs to be tested both in Firefox OS and Android.
Attachment #774835 - Flags: feedback?(marco.zehe)
Comment on attachment 774835 [details] [diff] [review]
Virtual cursor control refactor, fixes navigating in hidden frames.

Wow, this patch fixes a huge bunch of stuff!

On Android, it makes touching the screen more responsive. Multiple tabs, opening and closing, all work as expected.

On Firefox OS, pressing Home now consistently returns our screen reader to the home screen as well. All apps can be opened and closed, and the screen is always consistent. Also swiping through the different home screens works.
In addition, the keyboard is indeed accessible now!

Can't find anything wrong! :-)
Attachment #774835 - Flags: feedback?(marco.zehe) → feedback+
That is good news, this patch touches a lot, but makes things easier to understand. We may have unraveled some unnecessary steps in explore by touch that makes it snappier.
Attachment #774835 - Attachment is obsolete: true
Attachment #774852 - Flags: review?(maxli)
Attachment #774852 - Flags: review?(dbolter)
Comment on attachment 774852 [details] [diff] [review]
Bug 893153 - Virtual cursor control refactor, fixes navigating in hidden frames.

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

Sorry this is taking me a while... will need more time for my review.

::: accessible/src/jsat/Utils.jsm
@@ +234,5 @@
>  
>    inHiddenSubtree: function inHiddenSubtree(aAccessible) {
>      for (let acc=aAccessible; acc; acc=acc.parent) {
> +      let hidden = Utils.getAttributes(acc).hidden;
> +      if (hidden && JSON.parse(hidden)) {

Cool, should be faster.
Comment on attachment 774852 [details] [diff] [review]
Bug 893153 - Virtual cursor control refactor, fixes navigating in hidden frames.

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

I think the logic makes sense.

General formatting comment: some of the one line if statements have braces and some of them don't
Attachment #774852 - Flags: review?(maxli) → review+
Comment on attachment 774852 [details] [diff] [review]
Bug 893153 - Virtual cursor control refactor, fixes navigating in hidden frames.

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

r=me. Logic seems ok to me too.

::: accessible/src/jsat/content-script.js
@@ +212,1 @@
>  }

nit: I particularly like {}'s when a } immediately follows a single line if block. (here and elsewhere)
Attachment #774852 - Flags: review?(maxli)
Attachment #774852 - Flags: review?(dbolter)
Attachment #774852 - Flags: review+
Comment on attachment 774852 [details] [diff] [review]
Bug 893153 - Virtual cursor control refactor, fixes navigating in hidden frames.

Un-un-doing my review flag.
Attachment #774852 - Flags: review?(maxli) → review+
My bad. Sorry Max - thanks for fixing that.
(In reply to Max Li [:maxli] from comment #7)
> General formatting comment: some of the one line if statements have braces
> and some of them don't

(In reply to David Bolter [:davidb] from comment #8)
> nit: I particularly like {}'s when a } immediately follows a single line if
> block. (here and elsewhere)

Yeah, we should have braces on all if statements. Fixed.
Depends on: 894601
https://hg.mozilla.org/mozilla-central/rev/d9579a00bb93
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.