Closed Bug 638326 Opened 9 years ago Closed 9 years ago

getTextAtOffset line boundary may return more than one line

Categories

(Core :: Disability Access APIs, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla5
Tracking Status
blocking2.0 --- .x+

People

(Reporter: tbsaunde, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110302 Firefox/4.0b13pre
Build Identifier: 

between the nightly for june 10 2010 and june 11 2010 something changed such that on mailman pages displaying emails GetText() for the first line in the document frame will return the full page. 

Reproducible: Always
Blocks: orca
Keywords: regression
sometimes means from time to time or on always on some pages? If from time to time then bug 634202 should be a fix.
This is not a sometimes works sometimes doesn't issue, sites like mail.gnome.org/archives/gnome-accessibility-devel/2011-march/msg00000.html always have GetText() return the full text.
Can we have code snippet for that?
Trev, can you get me the two "built from" URLs from the June 10 and June 11 builds? You'll find them if you opena page called about:buildconfig. It#s right at the top after "Built from:".
Thanks!
Trev, please disregard my earlier comment! The regression window is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b84d0be52070&tochange=431eab8cf6ab

The biggie that landed was bug 541618, and a smaller crash bug 571246 also landed.
The bug doesn't show up in a build of the commit before 541618 but does show up with 541618.  Sorkov any ideas? that patch seems to have just renamed a lot, and changed nsItemNode to nsINode in nsAccessNode per the commit message.
the problem is nsIFrame::PeekOffset in GetRelativeOffset returns not accessible text node for end boundary.
the problem is in GetFirstAvailableAccessible change - http://hg.mozilla.org/mozilla-central/diff/117fe7a234cc/accessible/src/base/nsAccessible.cpp#l1.1522

The easiest fix would be rollback this change. Though it should introduce other problems like getting correct offset when CSS generated content is used.
If next rc is expected then we must fixed it, nominate for blocking for now.
Blocks: 541618
blocking2.0: --- → ?
Severity: normal → major
(In reply to comment #9)
> the problem is in GetFirstAvailableAccessible change -
> http://hg.mozilla.org/mozilla-central/diff/117fe7a234cc/accessible/src/base/nsAccessible.cpp#l1.1522
> 
> The easiest fix would be rollback this change. Though it should introduce other
> problems like getting correct offset when CSS generated content is used.

Why?
because DOM tree walker doesn't pick up nodes for generated content, so if we have like

[current node][CSS before node][accessible node]

then tree walker will return [accessible node] that should add [CSS before node] into text range automatically.
the point is we should use the same interface to find "closest" accessible to DOM node as we use to create an accessible tree.
OK thanks. Discovering this broke in June 2011 indicates we really need to get Orca community QA help (automated or otherwise) with our betas.

How broken is Orca with this bug?
Sorry tired here. I meant "Only now discovering that this broke back in June 2010..."
Alex: we've gone to RC, which means no more changes. Do you think that this bug is serious enough that we absolutely can not ship Firefox 4 without it?
Ok, I think backing out this part is less evil than keeping it. I don't see plain and simple correct fix. So taking into account importance of this bug I think we should rollback things. Note, bug 634202 should be a fix for this one.
(In reply to comment #16)
> Alex: we've gone to RC, which means no more changes. Do you think that this bug
> is serious enough that we absolutely can not ship Firefox 4 without it?

I think yes, let me give an analogy for it to help to understand how it is bad. Let's you listen some audio then you can rewind or skip the content, all you can listen from start to the end. This is bad experience when you forced to listen whole site and can't do that by parts.

From technical point of view (as I pointed above) I think we should rollback this part what should be trival.
Summary: GetGetText() sometimes returns whole text when asked for first line → getTextAtOffset line boundary may return more than one line
Blocks: 638684
Attached patch patch (obsolete) — Splinter Review
guys, if we are allowed to take it please land it if I'm not around.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #516807 - Flags: review?(bolterbugz)
Note, todo mochitest isn't regression from existing fx4 (I can't think of testcase to show the regression). That todo shows the old logic doesn't work in some cases (edge cases).
Comment on attachment 516807 [details] [diff] [review]
patch

My vote is this:
Given that this problem was only discovered a week ago or less by a person using Orca every day, and it's been around since June 2010, and the likelyhood of this special kind of markup appearing in many places being somewhat limited, I'd say:
1. If we have an RC2, definitely take this as a ride-along.
2. If we don't get an RC2, take it on a (the first?) dot release.

But no, do NOT hold the current RC process for this.
(In reply to comment #22)
> Comment on attachment 516807 [details] [diff] [review]
> patch
> 
> My vote is this:
> Given that this problem was only discovered a week ago or less by a person
> using Orca every day, and it's been around since June 2010,

that's correct but that if I get right Trevor isn't sure he used FX4 betas as his primary browser for daily usage.

> and the likelyhood
> of this special kind of markup appearing in many places being somewhat limited,

that's not special markup: whitespace not rendered text nodes, few months ago we have a lot of problems with these text nodes since they were presented on many sites.

Accessibility bugs are hard to discover since tester should remember what was to say if it gets broken. Presumably not many sites has these text nodes in the site beginning (what makes Orca to read whole site) but I would expect to see many sites that have this problem somewhere at the page. Though that's not scary but must be annoying.

I wouldn't vote for RC respin but I'd love to see this bug fixed in FX4.
(In reply to comment #20)
> try server build -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-eddfbc0eafc6

This patch fixes the original issues it also fixes an odd issue reading wikipedia articles line by line that I noticed, but marco and fer could never reproduce, for example see en.wikipedia.org/wiki/cofinality  orca would either get stuck  after the h1 but before the article, or skip way down to near the h5 for navigation at the end of the article.  So, while I'm not sure what the story with wikipedia is, I haven't looked at the markup it seems like this patch makes things a lot better.  I'll try to beat on this  build more in the morning.
Joanie, Fer, Ale, we'll try our best to figure out a way to land a fix safely, but now is the time to talk about impact. Do we have Orca FF4 users?
Per Marco's comment 22 and Surkov's comment 23
blocking2.0: ? → .x+
Whiteboard: [fx4-rc-ridealong]
Comment on attachment 516807 [details] [diff] [review]
patch

r=me. Optional follow up nits:

> nsAccessible::GetFirstAvailableAccessible(nsINode *aStartNode) const

The revert here looks correct. Might get a small perf hit right?

>+  nsCOMPtr<nsIDOMDocumentTraversal> trav = do_QueryInterface(document);
>+  NS_ENSURE_TRUE(trav, nsnull);

This will fail silently now; was that intended? (This is fine with me)

>+  <div id="hypertext3">line
>+<!-- haha -->
>+<!-- hahaha -->
>+<h6>heading</h6>
>+  </div>
>+
>+  <div id="hypertext4">line
>+<!-- haha -->
>+<!-- hahaha -->
>+<h6 role="presentation" class="gencontent">heading</h6>
>+  </div>

What's so funny? ;)
(Seriously, we could do <!-- add a whitespace node -->)

Looks safe, but given the timing I'm asking for additional looks from people.
Attachment #516807 - Flags: review?(neil)
Attachment #516807 - Flags: review?(bolterbugz)
Attachment #516807 - Flags: review+
Attachment #516807 - Flags: feedback?(marco.zehe)
Comment on attachment 516807 [details] [diff] [review]
patch

>+  nsCOMPtr<nsIDOMDocument> document;
>+  currentNode->GetOwnerDocument(getter_AddRefs(document));
>+  nsCOMPtr<nsIDOMDocumentTraversal> trav = do_QueryInterface(document);
do_QueryInterface(aStartNode->GetOwnerDoc()) probably also works.
Attachment #516807 - Flags: review?(neil) → review+
Comment on attachment 516807 [details] [diff] [review]
patch

f=me
Attachment #516807 - Flags: feedback?(marco.zehe) → feedback+
So, this is ZERO RISK, right?
Nothing is zero risk. I think this is close. Neil's review puts it even closer.
Philor has mostly starred the tbpl:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=eddfbc0eafc6

And has retriggered the 10.6 debug one...
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong][has reviewed patch][deemed not respin worthy in isolation]
Whiteboard: [fx4-rc-ridealong][has reviewed patch][deemed not respin worthy in isolation] → [fx4-rc-ridealong][has reviewed patch]
Note Phil (the orange expert) took the time to star all our tbpl try oranges so I'm confident there (note it only tested mochitest-o which is all that should be affected by the patch in a sane world).
Fer, as far as we know this bug will hit screen readers that don't use a virtual buffer the worst. Is there a potential workaround for Orca?
(In reply to comment #35)
> Fer, as far as we know this bug will hit screen readers that don't use a
> virtual buffer the worst. Is there a potential workaround for Orca?

Yes, orca users can switch from orca controlling the caret to firefox caret navigation. However it is not a trivial workaround (to know about it). In the other hand, as it is a regression from 3.6, users would know that there is more content in that page, and could try other methods to reach it.
This seems to be a more common issue than we'd realized, see bugzilla.gnome.org/show_bug.cgi?id=638211 and bugzilla.gnome.org/show_bug.cgi?id=625136 the later one even involves a mozilla.org page :).  It seems like there is another element in bugzilla.gnome.org/show_bug.cgi?id=638211 but this bug shows up there.  A final exibit seems to be some wordpress blogs near the end of the comments before the add comment fields, but that's completely uninvestigated so far. In any case, it seems to be fairly common.
Is this ready to be landed?
(In reply to comment #28)
> Comment on attachment 516807 [details] [diff] [review]
> patch
> 
> r=me. Optional follow up nits:
> 
> > nsAccessible::GetFirstAvailableAccessible(nsINode *aStartNode) const
> 
> The revert here looks correct. Might get a small perf hit right?

hard to say.

> >+  nsCOMPtr<nsIDOMDocumentTraversal> trav = do_QueryInterface(document);
> >+  NS_ENSURE_TRUE(trav, nsnull);
> 
> This will fail silently now; was that intended? (This is fine with me)

not silently, warning in console

> >+  <div id="hypertext4">line
> >+<!-- haha -->
> >+<!-- hahaha -->
> >+<h6 role="presentation" class="gencontent">heading</h6>
> >+  </div>
> 
> What's so funny? ;)
> (Seriously, we could do <!-- add a whitespace node -->)

it doesn't matter I guess :) though it's not about whitespace node, it's about comment node.
Attached patch patch to land (obsolete) — Splinter Review
Attachment #516807 - Attachment is obsolete: true
Attached patch patch to landSplinter Review
Attachment #521424 - Attachment is obsolete: true
landed - http://hg.mozilla.org/mozilla-central/rev/7252330fc716
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fx4-rc-ridealong][has reviewed patch]
Target Milestone: --- → mozilla2.2
verified works in 
Mozilla/5.0 (X11; Linux x86_64; rv:2.2a1pre) Gecko/20110329 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
blocking2.0: .x+ → ?
It was a hard decision, but we've decided to not take this for Macaw. It will be fixed in the next version of Firefox.

I'm putting this back into blocking:.x+, because if we do have to create another regular point release to 4.0 that will be the list we will be looking at first.
blocking2.0: ? → .x+
I find this bug very annoying as all HTML produced from converting a PDF with pdftohtml exhibits this bug. Is there any chance that this fix will ever be seen in firefox 4.0x as 4.0.x is currently the stable version of firefox being used in distributions.
You need to log in before you can comment on or make changes to this bug.