638326 - getTextAtOffset line boundary may return more than one line

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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

Comment 1

[alexander :surkov (:asurkov)]

sometimes means from time to time or on always on some pages? If from time to time then bug 634202 should be a fix.

Comment 2

[Trevor Saunders (:tbsaunde)]

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.

Comment 3

[alexander :surkov (:asurkov)]

Can we have code snippet for that?

Comment 4

[Marco Zehe (:MarcoZ)]

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!

Comment 5

[Fernando Herrera]

This is causing this orca bug:

https://bugzilla.gnome.org/show_bug.cgi?id=642996

Comment 6

[Marco Zehe (:MarcoZ)]

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.

Comment 7

[Trevor Saunders (:tbsaunde)]

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.

Comment 8

[alexander :surkov (:asurkov)]

the problem is nsIFrame::PeekOffset in GetRelativeOffset returns not accessible text node for end boundary.

Comment 9

[alexander :surkov (:asurkov)]

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.

Comment 10

[alexander :surkov (:asurkov)]

If next rc is expected then we must fixed it, nominate for blocking for now.

Comment 11

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

(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?

Comment 12

[alexander :surkov (:asurkov)]

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.

Comment 13

[alexander :surkov (:asurkov)]

the point is we should use the same interface to find "closest" accessible to DOM node as we use to create an accessible tree.

Comment 14

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

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?

Comment 15

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

Sorry tired here. I meant "Only now discovering that this broke back in June 2010..."

Comment 16

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

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?

Comment 17

[alexander :surkov (:asurkov)]

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.

Comment 18

[alexander :surkov (:asurkov)]

(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.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch

guys, if we are allowed to take it please land it if I'm not around.

Comment 20

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-eddfbc0eafc6

Comment 21

[alexander :surkov (:asurkov)]

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 22

[Marco Zehe (:MarcoZ)]

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.

Comment 23

[alexander :surkov (:asurkov)]

(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.

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to comment #20)
> try server build -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-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.

Comment 25

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

We need to make sure all the orange here is known:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=eddfbc0eafc6

Comment 26

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

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?

Comment 27

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

Per Marco's comment 22 and Surkov's comment 23

Comment 28

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

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.

Comment 29

[[e-mail]]

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.

Comment 30

[Marco Zehe (:MarcoZ)]

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

f=me

Comment 31

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

So, this is ZERO RISK, right?

Comment 32

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

Nothing is zero risk. I think this is close. Neil's review puts it even closer.

Comment 33

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

Philor has mostly starred the tbpl:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=eddfbc0eafc6

And has retriggered the 10.6 debug one...

Comment 34

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

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).

Comment 35

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

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?

Comment 36

[Fernando Herrera]

(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.

Comment 37

[Trevor Saunders (:tbsaunde)]

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.

Comment 38

[(no longer active)]

Is this ready to be landed?

Comment 39

[alexander :surkov (:asurkov)]

(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.

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch to land

Comment 41

[alexander :surkov (:asurkov)]

Attachment: patch to land

Comment 42

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/7252330fc716

Comment 43

[Trevor Saunders (:tbsaunde)]

verified works in 
Mozilla/5.0 (X11; Linux x86_64; rv:2.2a1pre) Gecko/20110329 Firefox/4.2a1pre

Comment 44

[LegNeato]

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.

Comment 45

[Michael Whapples]

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.