After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 546815 - guessLabelFromLine() is guessing text that is on other lines in FF3
guessLabelFromLine() is guessing text that is on other lines in FF3
Status: RESOLVED FIXED
Product: orca
Classification: Applications
Component: speech
2.23.x
Other All
: Normal normal
: 2.24.0
Assigned To: Joanmarie Diggs (IRC: joanie)
Orca Maintainers
Depends on: 535178
Blocks: 404403
 
 
Reported: 2008-08-07 14:30 UTC by Willie Walker
Modified: 2008-09-15 20:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
TestPage.html (1.91 KB, text/html)
2008-08-11 02:21 UTC, Ye Guo
  Details
revision 1 (32.70 KB, patch)
2008-09-14 07:56 UTC, Joanmarie Diggs (IRC: joanie)
committed Details | Review

Description Willie Walker 2008-08-07 14:30:29 UTC
Blocking bug for https://bugzilla.mozilla.org/show_bug.cgi?id=449550
Comment 1 Joanmarie Diggs (IRC: joanie) 2008-08-07 15:06:36 UTC
From the original bug report:

--------

1. Launch orca and enable speech.
2. Open the test page attached here.
3. Press <F7> to turn on caret browsing.
4. Press arrow key or Tab to focus on the checkbox 'wild animal'.

After speaking this checkbox 'wild animal', orca went on to speak the fourth
row in the test page. It shouldn't do that.

--------

The test page seems to be the htmlpage.html from our test files. I've confirmed this bug. And it's an odd one. But it's ours rather than theirs.  Unblocking and taking this one.
Comment 2 Joanmarie Diggs (IRC: joanie) 2008-08-07 16:10:24 UTC
Did some quick triage. guessLabelFromLine() is getting a little enthusiastic.  Looking at the code, not only can we fix this issue, we should be able to get a performance improvement because we'll already have the strings stored (once part 1 of bug 535178 is checked in).
Comment 3 Ye Guo 2008-08-11 02:21:21 UTC
Created attachment 116314 [details]
TestPage.html

Here's the test page. Sorry for my careless before.
Comment 4 Joanmarie Diggs (IRC: joanie) 2008-09-14 07:56:16 UTC
Created attachment 118684 [details] [review]
revision 1

This patch is a redo of guessLabelFromLine() which takes advantage of the stored strings which are now part of the line contents. In doing so, we seem to double (a bit more than double, actually) our performance in this method:

ncalls  tottime  percall  cumtime  percall
    46    0.042    0.001    3.108    0.068 guessLabelFromLine - old
    46    0.006    0.000    1.387    0.030 guessLabelFromLine - new

Oh, and it fixes the bug in the report too. :-) Along with a few other instances where we were not guessing the label on the line.

This patch also includes two new regression tests: One for this bug, and one from an older bug (bug 509809) involving guessing the label from the current line.

Regression testing showed one "regression": Turns out that in one of our tests, where the label to be guessed is on the next line, guessLabelFromLine() was incorrectly guessing it and we were lucking out -- basically it was another instance of this bug, but it just so happened that the bogus guess we were making coincided with the correct one. :-) I've marked that regression in the tests: It is a separate bug and should be treated (and fixed) as such.

Mike, please test.

We also need to decide if we want to include this in 2.24 or wait until 2.24.1. Thoughts?
Comment 5 Willie Walker 2008-09-15 13:48:27 UTC
(In reply to comment #4)
> We also need to decide if we want to include this in 2.24 or wait until 2.24.1.
> Thoughts?

It's a substantial change, but it is also pretty high impact in terms of usability.  So, if you feel comfortable with it and it tests well with Mike, I say commit.
Comment 6 Mesar Hameed 2008-09-15 14:32:39 UTC
Hi Joanie,

This patch seems to work well for me, thank you

One comment though, we seem to say ": text" is that intentional?

"5. Enter your Zip colon colon text"
I am running ibex with the firefox that comes with it. (last nights daily iso).




Comment 7 Joanmarie Diggs (IRC: joanie) 2008-09-15 14:42:55 UTC
(In reply to comment #6)
> "5. Enter your Zip colon colon text"

That's odd. I am getting only one colon which is not pronounced for me because I use punctuation level Some. When I switch to Most, the colon is pronounced exactly once. I wonder what's causing it for you? Also, does it only happen with this patch?
Comment 8 Mesar Hameed 2008-09-15 15:22:37 UTC
"colon text" happens with and without this patch, sorry for not saying this.
using espeak, and punctuation set to most.
both firefox 3.0 and nightly firefox.

I hear the following for the page:

"seperator"
"html form and widgets"
"blank"
"text field colon"
"enter your name colon colon text text field using type equals text"
"1 enter your address colon colon text text field"
"using size and maxlength"
"2 enter your city colon colon text 3 enter your state colon colon text 4 enter your country colon colon"
"text field using value text US text field using value" *
"5 Enter your Zip colon colon text"

below this point things seems to be working as expected.
The line that is starred is repeating "text field using value" is that intentional?
Comment 9 Joanmarie Diggs (IRC: joanie) 2008-09-15 15:24:36 UTC
It is not intentional, nor is it happening on my box. I'm not sure what to tell you. Mike, what are you seeing?
Comment 10 Mike Pedersen 2008-09-15 17:23:04 UTC
I'm finding this patch to work well.  I've been testing it all morning and can't reproduce Jon's issue.  
Comment 11 Joanmarie Diggs (IRC: joanie) 2008-09-15 20:04:47 UTC
Jon, while racking my brain trying to figure out what the difference might be, I guessed large print. So I increased my font size and decreased my browser size enough to cause the text to appear on the line above the entry, and sure enough I get two colons as well. That's from a different method, which is next on my list to revamp. Phew! :-) Thanks for pointing that out!

Patch committed to trunk. Closing as FIXED.