437607 - Clicking the "Skip to main content" link once works, second time fails to initiate a V cursor jump

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Steps to reproduce:
1. Open above URL.
2. Using JAWS, arrow to the "Skip to main content" link and press ENTER.

Result: Focus jumps to the "About Mozilla" heading.

3. Arrow around the page, then back to the "Skip to main content" link and press ENTER again.

Expected: Focus again jumps to the heading.
Actual: Nothing happens.

4. Focus one of the other links and press NUM PAD STAR to open its context menu. This will cause a focus jump to that link.
5. Press ESCAPE to close the menu.
6. Arrow back to "Skip to main content" and press ENTER.

Expected: Focus would jump to the heading.
Actual: Nothing happens.

Rob Gallo of Freedom Scientific says: "If you jump to a same page anchor a second time in a row Firefox doesn't fire the EVENT_SCROLLINGSTART event the 2nd time (that's the event we use to indicate
a same page jump)."

Comment 1

[Samuel Sidler (old account; do not CC)]

Marco, is this a regression from Firefox 2?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> Marco, is this a regression from Firefox 2?

No, in Firefox 2.0.0.17, Same Page links don't work at all yet with JAWS. So this is a feature that was made working in Firefox 3 but isn't working in all cases yet.

Comment 3

[Marco Zehe (:MarcoZ)]

Additional info from Rob: the first time you press a same page link, the scroll event is indeed sent, but the indicated object on the page
is not the point where the focus was supposed to be placed.

Comment 4

[Aaron Leventhal]

Marco, what does this part mean?
> but the indicated object on the page
is not the point where the focus was supposed to be placed.

Comment 5

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

Comment 6

[alexander :surkov (:asurkov)]

We fire scrolling start in the case of anchors, we don't fire scrolling start event in another cases, however we fire scroll end event whenever scrolling is finished. The funny thing is scrolling start event has anchor target as an event target, scrolling end event has document as an event target. What's expected actually?

Comment 7

[James Teh [:Jamie]]

(In reply to comment #6)
> We fire scrolling start in the case of anchors, we don't fire scrolling start
> event in another cases,
What cases do you refer to here?
> however we fire scroll end event whenever scrolling is
> finished.
Do you mean scrolling only with regard to anchors or do you mean even if a user scrolls the window; e.g. using the mouse?

> The funny thing is scrolling start event has anchor target as an
> event target, scrolling end event has document as an event target.
Ah. I always wondered why we had to use scrolling start instead of scrolling end. It always made more sense to me to use scrolling end.

> What's
> expected actually?
It makes more sense to me to fire scrolling end on the anchor target.

Will scrolling end get fired even if we call IAccessible2::scrollTo()? This would be tricky, as we'd then have to figure out whether we caused a scrolling end event ourselves.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > We fire scrolling start in the case of anchors, we don't fire scrolling start
> > event in another cases,
> What cases do you refer to here?

Any other case when scrolling happens :) for example if user scrolls document by keyboard or mouse.

> > however we fire scroll end event whenever scrolling is
> > finished.
> Do you mean scrolling only with regard to anchors or do you mean even if a user
> scrolls the window; e.g. using the mouse?

correct if I read the code right :)

> > The funny thing is scrolling start event has anchor target as an
> > event target, scrolling end event has document as an event target.
> Ah. I always wondered why we had to use scrolling start instead of scrolling
> end. It always made more sense to me to use scrolling end.

I agree scrolling end sounds like more logically correct here.

> > What's
> > expected actually?
> It makes more sense to me to fire scrolling end on the anchor target.

Yes, since ATs doesn't use scrolling end event then we can change this.

> Will scrolling end get fired even if we call IAccessible2::scrollTo()? This
> would be tricky, as we'd then have to figure out whether we caused a scrolling
> end event ourselves.

I think yes, it should. We could filter it I think.

Currently if I get right ATs expect scrolling event whenever user clicks on a link and document is scrolled as result. They don't want it if user scrolls screen by mouse or by keyboard or if IAccessible2::scrollTo is called. However if page is scrolled programmatically then do ATs expect scrolling events?

Comment 9

[James Teh [:Jamie]]

(In reply to comment #8)
> Currently if I get right ATs expect scrolling event whenever user clicks on a
> link and document is scrolled as result.
Correct. I seem to have gotten a little sidetracked here. The issue is that scrolling start doesn't get fired if a user activates a particular same page link after it has already been activated once. We can fix scrolling end, but that won't work for existing ATs and will result in different behaviour because scrolling end fires in other cases as well.

> They don't want it if user scrolls
> screen by mouse or by keyboard
This is debatable. I think it's correct to say that current ATs don't expect this, but I wonder if it could be useful for some people. AGain, though, this is probably outside the scope of this bug.

> or if IAccessible2::scrollTo is called.
Probably not, as you can't trust that you will get a scroll event in this case (the page might not have actually scrolled if the object was already on screen).

> However
> if page is scrolled programmatically then do ATs expect scrolling events?
Probably outside the scope of this bug, but are there good use cases for this?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> > They don't want it if user scrolls
> > screen by mouse or by keyboard
> This is debatable. I think it's correct to say that current ATs don't expect
> this, but I wonder if it could be useful for some people.

But I can't see a way to bring any changes to not broke existing ATs.

> > However
> > if page is scrolled programmatically then do ATs expect scrolling events?
> Probably outside the scope of this bug, but are there good use cases for this?

Programmatic link I guess like <button onclick="scrollToSomeElement()">. It's probably wild for HTML but it might be used for XUL apps for example.

Comment 11

[Marco Zehe (:MarcoZ)]

Before this debate gets any further, let's fix the problem at hand and keep existing behavior. There are so many versions of screen readers out in the wild that rely on current behavior that changing it now to use SCROLLING_END event would break them all. We can do this change in a follow-up bug IF we get feedback that this is actually more useful from all involved parties.

So for this bug, please stick to using SCROLLING_START, and fix the problem at hand that if you click a Same Page link twice, it always gets fired when expected to. Thanks!

Comment 12

[alexander :surkov (:asurkov)]

Marco, debates like this are really helpful to understand HOW it should be fixed. I never touched this code previously so I need to realize what is correct. Also I think nobody is going to break current arrangement between Firefox and AT in the meantime. However I don't decline this is might be a subject to change in the future.

Comment 13

[Marco Zehe (:MarcoZ)]

Right, and I agree we should change it to something more useful. But not before at least Firefox 3.7 is out. We can target this change at the next major releases of ATs for next year.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Cool. If we do fix this bug minimally, let's file a follow up for the better long term solution.

Comment 15

[alexander :surkov (:asurkov)]

taking, bug 519303 should be fixed as well

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 405968 [details] [diff] [review]
patch

r=me for the a11y code with some nits.

>   /**
>+   * Notify an accessibility that anchor jump has been accomplished to the given
>+   * target. Used by layout.
>+   */

Nit: remove "an".

> NS_IMETHODIMP
>+nsAccessibilityService::NotifyOfAnchorJumpTo(nsIContent *aTarget)
>+{
>+  nsCOMPtr<nsIDOMNode> targetNode(do_QueryInterface(aTarget));
>+
>+  nsCOMPtr<nsIAccessible> targetAcc;
>+  GetAccessibleFor(targetNode, getter_AddRefs(targetAcc));

Aside: Maybe we should add GetAccessibleFor(nsIContent) API

>+  if (!targetAcc) {

Nit: can you add a comment about what code block this does? (GetAccessibleInParentChain)


>+    text text text text text text text text text text text text text text <br>

Why not just one:
text<br>
?

>     function doTest()
>     {
>-      if (!WIN) {

Nice.

>+    var gURL =
>+      "chrome://mochikit/content/a11y/accessible/events_scroll.html#link1";

Nit: I don't think you need this as a g variable.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh and please run on tryserver :)

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> >+  nsCOMPtr<nsIDOMNode> targetNode(do_QueryInterface(aTarget));
> >+
> >+  nsCOMPtr<nsIAccessible> targetAcc;
> >+  GetAccessibleFor(targetNode, getter_AddRefs(targetAcc));
> 
> Aside: Maybe we should add GetAccessibleFor(nsIContent) API

perhaps, dunno, subject of investigation

> 
> >+    text text text text text text text text text text text text text text <br>
> 
> Why not just one:
> text<br>
> ?

I like when there are many symbols on the screen :)

> 
> >+    var gURL =
> >+      "chrome://mochikit/content/a11y/accessible/events_scroll.html#link1";
> 
> Nit: I don't think you need this as a g variable.

ok

Comment 20

[alexander :surkov (:asurkov)]

Robert, ping?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Oh and please run on tryserver :)

It looks ok, it sounds only random failures, build available at http://build.mozilla.org/tryserver-builds/[e-mail]-try-7c1c24a0c130

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't you just use 'content' instead of anchorTarget?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Why don't you just use 'content' instead of anchorTarget?

We need to fire an event for the element we scroll to. 'content' variable might be changed when caret position is set (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#3884).

Comment 24

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/249a47402dfa

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 405968 [details] [diff] [review]
patch

This bug is very important for NVDA screen reader  (see bug 519303 comment #10 from NVDA developer).

The patch was landed  two months ago and no regressions were caught. Also it's covered by mochitests suite.

Comment 26

[Marco Zehe (:MarcoZ)]

I second this request!

Alexander, can you check if the patch applies cleanly and prepare a patch for 1.9.2 if necessary so we don#t have any delays should we get approval?

Comment 27

[alexander :surkov (:asurkov)]

I will.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch1.9.2

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch1.9.2

added missed files

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 424978 [details] [diff] [review]
patch1.9.2

Sorry, we failed you here and didn't see this quickly enough. We'll try again for 1.9.2.3

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 424978 [details] [diff] [review]
patch1.9.2

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

(the above is boilerplate, but I noticed you're changing a UUID and interface on the branch -- we don't allow that just in case some add-on is using it. You'll need to create an nsIAccessibilityService_1_9_2_BRANCH for the additional methods)

Comment 32

[Marco Zehe (:MarcoZ)]

Alex, can you deal with the remark from comment #31 and submit a new patch and request approval 1.9.2.7 on it? I think we still need this.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> Alex, can you deal with the remark from comment #31 and submit a new patch and
> request approval 1.9.2.7 on it? I think we still need this.

Marco, the request flag sequentially moves from 1.9.2.1 and now we reached 1.9.2.7. Usually a11y patches don't get into branches except top crashes. I'm just a bit skeptic to waste my time to import patch to 1.9.2 because it won't be taken most likely.

Comment 34

[Marco Zehe (:MarcoZ)]

We should check with the NVDA people since they have to deal with the changed behavior between 3.6 and trunk currently. Note that we *have* a patch against 1.9.2 on this bug already which, if I understand correctly, just needs a modification to not change the UUID.

Comment 35

[James Teh [:Jamie]]

The current stable release of NVDA (2010.1) handles scrolling in Firefox 3.6 correctly. It's probably fair to expect NVDA users to upgrade, so this isn't critical for NVDA any longer.