Last Comment Bug 691734 - After bug 673958, opening a page with a defined anchor target in the address no longer causes NVDA to jump directly to that anchor.
: After bug 673958, opening a page with a defined anchor target in the address ...
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
https://wiki.mozilla.org/Accessibilit...
: 628673 (view as bug list)
Depends on: 591639 693658
Blocks: a11ynext 673958
  Show dependency treegraph
 
Reported: 2011-10-04 06:21 PDT by Marco Zehe (:MarcoZ)
Modified: 2012-01-15 11:16 PST (History)
4 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.70 KB, patch)
2011-10-05 07:25 PDT, alexander :surkov
mzehe: review-
Details | Diff | Splinter Review
patch2 (12.68 KB, patch)
2011-10-06 01:07 PDT, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2011-10-04 06:21:55 PDT
After the big focus rework bug 673958, opening a page like the one provided in the URL above no longer causes NvDA to directly jump to the anchor target and start reading from there. Instead, virtual cursor always starts at the top. Before the build bug 673958 became checked into, 2009-09-28, this was the case, with bug 673958 in, it no longer is. Jumping from a link to an anchor target via link activation works fine, though. So I can jump from the table of content to any of the anchor targets on the same page. Only if it is opened from the URL bar and initially has an anchor target specified, it does not work.
Comment 1 James Teh [:Jamie] 2011-10-04 15:02:03 PDT
Similar to bug 628673. This could be because the scrollingStart event is fired before the focus event or it could just be a change in timing. If the former, it should be fixed. If the latter, we need bug 617544.
Comment 2 James Teh [:Jamie] 2011-10-04 17:01:42 PDT
Testing with AccProbe, it looks like scrollingStart is being fired before the focus event, so this probably should be fixed.
Comment 3 alexander :surkov 2011-10-04 19:40:40 PDT
(In reply to James Teh [:Jamie] from comment #1)
> Similar to bug 628673. This could be because the scrollingStart event is
> fired before the focus event or it could just be a change in timing. If the
> former, it should be fixed. If the latter, we need bug 617544.

Don't we want a bug 617544 for everything? Is the point that scrollingStart before focus event makes no sense? Taking care about events ordering should require us to introduce new not trivial code hunks.
Comment 4 James Teh [:Jamie] 2011-10-04 19:48:01 PDT
(In reply to alexander surkov from comment #3)
> Don't we want a bug 617544 for everything?
We do definitely want that regardless, yes.

> Is the point that scrollingStart
> before focus event makes no sense?
Correct: I don't think scrollingStart before focus makes sense. If bug 617544 is fixed, this won't matter any more. However, I still don't think it makes sense. :)
Comment 5 alexander :surkov 2011-10-05 00:56:38 PDT
Ok, I think we could implement IA2 proposal on Gecko side internally (i.e. implement functionality not IA2 interfaces) and fire scrollingStart event whenever documents gets focus and it has anchor target and then null out the anchor target if something else within a document gets the focus.
Comment 6 alexander :surkov 2011-10-05 07:25:30 PDT
Created attachment 564843 [details] [diff] [review]
patch
Comment 8 Marco Zehe (:MarcoZ) 2011-10-05 22:09:25 PDT
Comment on attachment 564843 [details] [diff] [review]
patch

r- because this try-server build does not fix the problem for me at all. The behavior is just as observed with a regular nightly build. Jamie, can you see if you get any different events now than with a regular nightly?
Comment 9 alexander :surkov 2011-10-05 22:15:09 PDT
I think the answer is in bug 628673 comment #9:

> NVDA starts to read the page from beginning anyway. Do you create vb
> synchronously when you handle document load event or store events until vb is
> created after you scheduled its creation?
Ack. I forgot we create it asynchronously after receiving the event and don't store events for the buffer. The theory is that generally, we don't need any events for the buffer until the buffer is actually created. This is very much a timing issue. This is another reason for needing bug 617544: there shouldn't be any event state that can't be determined by making a query. I guess it must just be working for Marco in 3.6 because the timing is different.

We might be able to hack around this in NVDA, but honestly, I think it might just be worth waiting until bug 617544 can be done, which I imagine will be after FF4. The reason is that even if this is fixed, it'll still break if you move focus out of the document while it is loading. Also, this rarely works for me even in Firefox 3.6, probably because of the above timing issue.

It's probably still more correct to fire this event after the document loses the busy state.
Comment 10 James Teh [:Jamie] 2011-10-05 22:44:35 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #8)
> Jamie, can you
> see if you get any different events now than with a regular nightly?
I can confirm that the events are now fired in the correct order.

(In reply to alexander surkov from comment #9)
> I think the answer is in bug 628673 comment #9:
Correct. This is now purely a timing issue.

Recent changes in NVDA mean that I might be able to make this behave a little better now. In short, it's now possible to catch specific events even before the buffer is rendered. However, this still doesn't solve the problem of a document that finishes loading before NVDA even knows about it; e.g. loading a document before NVDA is started or restarting NVDA after a document has been loaded. For that, we need bug 617544.
Comment 11 James Teh [:Jamie] 2011-10-06 00:46:50 PDT
(In reply to James Teh [:Jamie] from comment #10)
> However, this still doesn't solve the problem
> of a document that finishes loading before NVDA even knows about it; e.g.
> loading a document before NVDA is started or restarting NVDA after a
> document has been loaded.
I read the patch and it looked like scrollingStart gets fired on the anchor whenever the document gets focus. Subsequent testing confirms this. While this does actually fix the problem I mentioned, it causes a rather severe regression. Because scrollingStart gets fired every time focus moves away from and returns to the document, the user will keep losing their position in the document, as NVDA moves the cursor in response to scrollingStart events.
Comment 12 James Teh [:Jamie] 2011-10-06 00:49:00 PDT
The ideal solution would be to only fire the initial scrollingStart event when the document *first* gains focus; i.e. not on every focus event.
Comment 13 alexander :surkov 2011-10-06 00:49:46 PDT
I can fix it. But technically what is requirement to scrolling start event? Should it be fired after focus or after document is loaded?
Comment 14 James Teh [:Jamie] 2011-10-06 00:51:44 PDT
(In reply to alexander surkov from comment #13)
> what is requirement to scrolling start event?
> Should it be fired after focus or after document is loaded?
"Loaded" is now a fairly vague term and it means nothing to NVDA now anyway. The best we can say is after focus.
Comment 15 alexander :surkov 2011-10-06 01:07:55 PDT
Created attachment 565154 [details] [diff] [review]
patch2
Comment 16 Marco Zehe (:MarcoZ) 2011-10-06 06:55:44 PDT
Comment on attachment 565154 [details] [diff] [review]
patch2

r=me with one nit/question:
>+    const CC = Components.classes;

Where do you actually use this const? I don't see it in the file. A left-over?
Comment 17 alexander :surkov 2011-10-06 07:26:58 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #16)
> Comment on attachment 565154 [details] [diff] [review] [diff] [details] [review]
> patch2
> 
> r=me with one nit/question:
> >+    const CC = Components.classes;
> 
> Where do you actually use this const? I don't see it in the file. A
> left-over?

that goes from tabbrowser bindings, it relies on variables and constants outside it
Comment 18 alexander :surkov 2011-10-07 22:18:16 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/c664f85010b0
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 07:37:21 PDT
https://hg.mozilla.org/mozilla-central/rev/c664f85010b0
Comment 20 alexander :surkov 2011-10-10 20:07:20 PDT
*** Bug 628673 has been marked as a duplicate of this bug. ***
Comment 21 James Teh [:Jamie] 2011-10-10 20:18:51 PDT
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111010 Firefox/10.0a1.

FYI, with this bug and NVDA main r4719, NVDA will always move to the right position as long as the document is loaded while NVDA is running. We still need bug 617544 to fix the case where a document is loaded before NVDA is started (or restarted).

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