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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
mozilla1.9.3a1
x86
Windows XP
access
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 ?
wanted1.9.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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)."
Flags: wanted1.9.1?
(Reporter)

Updated

10 years ago
Flags: wanted1.9.1? → wanted1.9.0.x?
Marco, is this a regression from Firefox 2?
Flags: wanted1.9.1?
(Reporter)

Comment 2

10 years ago
(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.
(Reporter)

Comment 3

9 years ago
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

9 years ago
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.
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.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
(Assignee)

Comment 6

9 years ago
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

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
(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

9 years ago
(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?
(Assignee)

Comment 10

9 years ago
(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.
(Reporter)

Comment 11

9 years ago
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!
(Assignee)

Comment 12

9 years ago
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.
(Reporter)

Comment 13

9 years ago
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.
Cool. If we do fix this bug minimally, let's file a follow up for the better long term solution.
(Assignee)

Comment 15

8 years ago
taking, bug 519303 should be fixed as well
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
(Assignee)

Comment 16

8 years ago
Created attachment 405968 [details] [diff] [review]
patch
Attachment #405968 - Flags: superreview?(roc)
Attachment #405968 - Flags: review?(bolterbugz)
Attachment #405968 - Flags: review?(bolterbugz) → review+
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.
Oh and please run on tryserver :)
(Assignee)

Comment 19

8 years ago
(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
(Assignee)

Comment 20

8 years ago
Robert, ping?
(Assignee)

Comment 21

8 years ago
(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/surkov.alexander@gmail.com-try-7c1c24a0c130
Why don't you just use 'content' instead of anchorTarget?
(Assignee)

Comment 23

8 years ago
(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).
Attachment #405968 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 24

8 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/249a47402dfa
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.2b1
Target Milestone: mozilla1.9.2b1 → mozilla1.9.3a1
(Assignee)

Comment 25

8 years ago
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.
Attachment #405968 - Flags: approval1.9.2.1?
(Reporter)

Comment 26

8 years ago
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?
(Assignee)

Comment 27

8 years ago
I will.
(Assignee)

Comment 28

8 years ago
Created attachment 424973 [details] [diff] [review]
patch1.9.2
Attachment #424973 - Flags: approval1.9.2.1?
(Assignee)

Comment 29

8 years ago
Created attachment 424978 [details] [diff] [review]
patch1.9.2

added missed files
Attachment #424973 - Attachment is obsolete: true
Attachment #424978 - Flags: approval1.9.2.1?
Attachment #424973 - Flags: approval1.9.2.1?
(Assignee)

Updated

8 years ago
Attachment #405968 - Flags: approval1.9.2.1?
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
Attachment #424978 - Flags: approval1.9.2.2? → approval1.9.2.3?
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)
Attachment #424978 - Flags: approval1.9.2.4?
(Reporter)

Comment 32

8 years ago
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.
(Assignee)

Comment 33

8 years ago
(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.
(Reporter)

Comment 34

8 years ago
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

8 years ago
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.
(Reporter)

Updated

7 years ago
Depends on: 657628
Blocks: 52352
You need to log in before you can comment on or make changes to this bug.