Closed
Bug 691734
Opened 13 years ago
Closed 13 years ago
After bug 673958, opening a page with a defined anchor target in the address no longer causes NVDA to jump directly to that anchor.
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
VERIFIED
FIXED
mozilla10
People
(Reporter: MarcoZ, Assigned: surkov)
References
(Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
Testing with AccProbe, it looks like scrollingStart is being fired before the focus event, so this probably should be fixed.
Assignee | ||
Comment 3•13 years ago
|
||
(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•13 years ago
|
||
(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. :)
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #564843 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 7•13 years ago
|
||
Reporter | ||
Comment 8•13 years ago
|
||
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?
Attachment #564843 -
Flags: review?(marco.zehe) → review-
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #564843 -
Attachment is obsolete: true
Attachment #565154 -
Flags: review?(marco.zehe)
Reporter | ||
Comment 16•13 years ago
|
||
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?
Attachment #565154 -
Flags: review?(marco.zehe) → review+
Assignee | ||
Comment 17•13 years ago
|
||
(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
Assignee | ||
Comment 18•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Comment 21•13 years ago
|
||
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).
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•