171003 - when tabbing nav is "all elements", need to hit tab 2x for linked images

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[sairuh (rarely reading bugmail)]

when tabbing nav is "all elements" ("accessibility.tabfocus", 7), i need to hit
tab twice for linked images. this seems to be regression from the fix for bug
140612.

i'll attach a simple test case soon.

not sure if bug 170921 might be related, though.

Comment 1

[Aaron Leventhal]

This is a major accessibility bug.

Comment 2

[sairuh (rarely reading bugmail)]

1. go to http://hopey.mcom.com/tests/linked-image.html
2. start with focus in the urlbar --hit accel-L if needed.
3. start hitting shift-tab to cycle backwards(*)

* 1st hit: "boring text link" link.
* 2nd hit: the linked image is focused --it becomes faded in appearance-- and if
you bring up the context menu (shift-F10), you'll get one for a linked-image.
* 3rd hit: the linked image is focused again --but it appears *reversed*-- and
if you bring up the context menu (shift-F10), you'll get one for a *link*.
* 4th hit: the document's frame is focused.

(*)for some bizarre reason, going fwd with tab doesn't display this behavior. is
this another image selection issue, perhaps?

Comment 3

[sairuh (rarely reading bugmail)]

the image selection stuff was checked into the trunk around 9/11-12, whereas
akkana's fix for 140612 went in on 9/18. will see how this bug relates...

Comment 4

[sairuh (rarely reading bugmail)]

looking at a trunk build from 9/13, i don't see this bug. when i run the test in
comment 2, when focus is on the linked image, i get the context menu for only a
link (not linked image). and, i need only hit the tab (ie, shift-tab) key once
to move focus away from the linked image.

i'm not sure if this is purely a regression due to bug 140612, or if the fix to
140612 might be exposing a bug in the image selection landing.

mike or simon, would you know how to further diagnose this?

Comment 5

[Akkana Peck]

Attachment: Test case

Comment 6

[Akkana Peck]

Attachment: Simple fix, two lines with diff -w

Here's the fix.  Seeking review.

Comment 7

[Akkana Peck]

Attachment: Patch without -w

Here's the patch including the reindenting for the nesting level that was
removed (harder to read).

Comment 8

[Akkana Peck]

Comment on attachment 103779 [details] [diff] [review]
Simple fix, two lines with diff -w

Discussed this with sairuh -- apparently the selection is supposed to be on the
image, not the link containing the image (so the context menu should have both
link and image stuff in it, and the image should appear selected rather than
just outlined).

Comment 9

[sairuh (rarely reading bugmail)]

side note: also see bug 176281, since we don't currently focus unlinked images
in the tabbing cycle.

Comment 10

[Akkana Peck]

Comment on attachment 103779 [details] [diff] [review]
Simple fix, two lines with diff -w

It turns out that before my change, we only focused the links anyway, not the
images.  So this patch is still relevant, and cures the regression I
introduced. Figuring out who changed this since moz1.0 (which did only focus
links) is a separate issue.  Let's cure the regression first.  Aaron, can you
review?

Comment 11

[Aaron Leventhal]

Comment on attachment 103779 [details] [diff] [review]
Simple fix, two lines with diff -w

Makes sense. r=aaronl

Comment 12

[Aaron Leventhal]

Is this a better fix? I think the image map links shouldnt' get focused if you
have that setting where links don't get focus.

         else if (nsHTMLAtoms::img==tag) {
-          // Images are treated like links for tab focus purposes.
-          disabled = !(sTabFocusModel & eTabFocus_linksMask);
-          if (!disabled) {
+          if (sTabFocusModel & eTabFocus_linksMask) {
             nsCOMPtr<nsIDOMHTMLImageElement> nextImage(do_QueryInterface(child));
             nsAutoString usemap;
             if (nextImage) {

Comment 13

[Akkana Peck]

Attachment: Aaron's patch: include

Yes, much better -- good point, Aaron!	Here it is as a patch.	Bryner, could
you sr?  I'll also attach a test case that includes an imagemap, adapted from
one Aaron sent me.

Comment 14

[Akkana Peck]

Attachment: image for an imagemap test

Comment 15

[Akkana Peck]

Attachment: more general test case

Comment 16

[Akkana Peck]

Comment on attachment 103910 [details] [diff] [review]
Aaron's patch: include 

r=akkana on Aaron's patch.

Comment 17

[Aaron Leventhal]

Comment on attachment 103910 [details] [diff] [review]
Aaron's patch: include 

r=aaronl

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 103910 [details] [diff] [review]
Aaron's patch: include 

sr=bryner

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 103910 [details] [diff] [review]
Aaron's patch: include 

a=dbaron for trunk checkin.  It seems like it might be worth a comment as to
why you don't set |disabled| for this case, though.

Comment 20

[Akkana Peck]

> It seems like it might be worth a comment as to
> why you don't set |disabled| for this case, though.

Good question -- it wasn't obvious to me either.  Turns out that if the code
matches an imagemap, it returns directly from there rather than from the clause
at the end of the loop that checks for !disabled.  I've added a comment saying so.

Comment 21

[sairuh (rarely reading bugmail)]

vrfy'd fixed --no longer get the 2x tabbing strangeness. tested with
2002.10.29.08 comm trunk builds on mac 10.2.1, linux rh7.2 and win2k.

thanks, akkana!

Comment 22

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381