GNOME Bugzilla – Bug 516174
FF line navigation needs to be more accurate
Last modified: 2008-07-22 19:33:49 UTC
As part of the performance enhancements, I completely redid line navigation. While things got significantly faster, things got less accurate for pages with unusual markup. Sadly, over the past few weeks, I've come to the conclusion that unusual markup is the norm rather than the exception. While a 100% character-by-character examination of the document is no longer necessary, the almost complete abandonment of find{Next,Previous}CaretInOrder() was a mistake. We need to rely upon it more. My bad. Patch to follow.
Created attachment 105117 [details] [review] proposed patch This patch: * Solves several bugs that I was aware of but hadn't entered * Seems to solve the garmin bug (bug #513323) * Seems to solve the schwabb bug (bug #513321) * Hopefully solves the google bug (bug #515571) * Simplifies the code quite a bit * Passes all of the regression tests with the following exceptions: - A whitespace character is added/removed - Wacky nested layout tables are different Given that the wacky nested layout table in question is indeed a wacky case, and given that this approach solves far more things in exchange, my proposal is that we open a special bug for the wacky nested layout table and do special handling for it rather than attempt to add in special handling for all of the other aforementioned bugs -- plus other accuracy-related bugs about which we are currently unaware, but which no doubt exist. In terms of performance, while this change does make things *a tad* more slow than they were post performance enhancements, we're still quite a bit more performant than before all the changes. So instead of being, say, 6 times as fast, we're 5 times as fast. Personally, I think it is worth going with the simplified code and the accuracy. I have another idea as to where we can pick up performance with line nav which will hopefully make up for the difference. Mike, please test this at your earliest convenience. I'm sure there are other issues remaining to be solved. We can open new bugs for them. <smile> What I want to know w.r.t. this bug is the following: Does this make things worse? If the answer is "no", I would like to check it in and base other fixes, improvements, and regression tests on it. Thanks!
Created attachment 105177 [details] [review] added special handling for newlines in t-bird Mike pointed out via IM that this patch (so far) seems fine with FF but in certain messages gets hung up. Here is what seems to be going on: In web content, a forced line break within a paragraph is accomplished by a line break tag. We can get hung up on those, so we check for them. In a plain text message in Thunderbird forced line breaks are just newline characters. So this version leaves Gecko.py as it was in the previous patch and overrides isLineBreakChar() in the Thunderbird script. This seems to solve it for me. Mike please test. Thanks!
Created attachment 105188 [details] [review] change handling for ARIA Scott mentioned that ARIA page tabs were in a bad way -- and they admittedly have been since before this patch. But might as well fix 'em now. :-) He also indicated that Mike wanted all page tabs to appear in the braille context even though that's not what we do in Gtk+. I'm cool with that. The question is, what should occur if the page tabs occupy more than one line? Show all of them, or just show the ones on the current line? I took a guess and went with "show the ones on the current line" since the code in question is to present the current line. If that's not right, let me know. :-) In the meantime.... Scott: Here is what I've done: In the code that gets the line contents, I was getting the next/previous object and offset via find{Next,Previous}CaretInOrder(). As a result, we were picking up the whitespace characters within the parent page tab list. This version of the patch does the same thing -- unless isAriaWidget() in which case it instead gets the next/previous object via find{Next,Previous}Object() and sets the offset to 0. It seems to work for the page tab issue, based on my understanding of what is desired. If you could let me know: 1) If I now have page tabs right, and 2) What Aria stuff I just broke as a result of this change :-) That would be great. Between the two of us, we'll get it all sorted out. Thanks!! I need to re-run the regression tests on this patch still. Will do that now.
This patch seems to be working well. I'd be in favor of getting it checked in. thanks
Created attachment 105192 [details] [review] just.... one.... more.... ARIA.... tweak :-) In running the full suite of regression tests, I noticed that we had a number of "section section section"s in the braille for certain ARIA widgets. Scott since emailed me to tell me that might go away all on its own. Before I knew that, however, I added one more ARIA tweak and re-ran the tests. This got some of the extra sections. It also seemed to make other bogusity go away here and there. Because the new tweak basically says "do this foo unless it's an ARIA widget," and because I've re-run the tests, and because it pylints, and because Mike said he's in favor of it being checked in, I'm going to go ahead and do so. :-) Scott, I think some of the additional issues (duplicate objects) may go away when I do the whitespace stripping (bug 515804). <fingers crossed>
Patch officially in trunk. Moving to pending.
Created attachment 105204 [details] [review] add handing for line breaks in preformatted text We need to handle line breaks in preformatted text as well. This patch has been functionally tested (both FF and t-bird) and pylinted. I'm running the set of FF regression tests atm so this patch has not yet been committed. Removing the pending until we're pending again. :-)
Okay, it passed the regression tests. Back to pending it goes. With any luck there won't be anything else earth shattering and we can open new, specific bugs for the rest of the work that needs doing.
As we are opening other bugs for the stickage I think this one is good to close.
Thanks Mike. I think I've already fixed one stickage issue. Waiting for pylint and regression tests, but stay tuned. In the meantime, closing this one as FIXED.