Last Comment Bug 307545 - 4 ways to use CSS to make win32-based screen readers crawl
: 4 ways to use CSS to make win32-based screen readers crawl
Status: RESOLVED FIXED
[ETA: let's fix this the right way in...
: access, fixed1.8
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Aaron Leventhal
:
Mentors:
http://www.visuaide.com/HW/en/index.asp
Depends on: 297074 309099 309101
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-08 11:57 PDT by Aaron Leventhal
Modified: 2005-09-30 21:40 PDT (History)
11 users (show)
mtschrep: blocking1.8b5-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Never use PaintTextSlowly when a win32 screen reader is present (2.21 KB, patch)
2005-09-16 14:02 PDT, Aaron Leventhal
roc: review-
Details | Diff | Splinter Review

Description Aaron Leventhal 2005-09-08 11:57:51 PDT
1. Load Firefox and Window-Eyes 5.5 beta from http://www.gwmicro.com
2. http://www.visuaide.com/HW/en/index.asp
3. Tab through links

If you remove text-align: justify the perf problem goes away.

GW Micro says that there are too many DeleteDC( ) and ExtTextOut( ) calls, which
is bogging down their OSM processing.

Perhaps there is a way to use fewer DC's. Other than that, no sure what we can
do other than not use this style when a screen reader is running. We can detect
that.
Comment 1 parente 2005-09-11 07:09:33 PDT
Does the problem occur when the user turns off stylesheets on the page? (View, 
Page Style, No Style)
Comment 2 Aaron Leventhal 2005-09-13 07:20:51 PDT
Yes that fixes it, as does just turning off that one style.

We don't want to turn of style sheets completely.
Styles like display: none are used by DHTML pages and impact what gets exposed
to the screen reader.
Comment 3 Aaron Leventhal 2005-09-14 08:52:27 PDT
The problem is actually all the draws that occur when you draw the focus
outline. Bill says there were about 1000 text draws in a second when the focus
outline was drawn.

That seems like a perf problem in our layout code. Roc, David, what do you think?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-09-14 09:06:44 PDT
I have a question.
Is ExtTextOut API used on cairo instead of gfx?

I think that most simple fix is that we should not use sTextFrame::PaintTextSlowly.
# nsTextFrame::PaintTextSlowly is used for smallcaps, wordspacing, letterspacing
and justification.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-14 14:44:48 PDT
(In reply to comment #3)
> The problem is actually all the draws that occur when you draw the focus
> outline. Bill says there were about 1000 text draws in a second when the focus
> outline was drawn.

We draw one rectangle for each dot in the outline.
Comment 6 Aaron Leventhal 2005-09-14 15:51:43 PDT
> We draw one rectangle for each dot in the outline.

Yes, but he was talking about text draws.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-14 15:56:56 PDT
You're saying that focusing an element does 1000 text draws? That seems a little
wrong. Is it true on a page with only a few text elements?
Comment 8 William Smith 2005-09-15 09:15:13 PDT
The fewer justified text elements, the fewer text draws.

Also, I've seen some evidence that we might get overloaded with 
stretchblt's too, but I'm not as confident on that.

The problematic textouts seem almost exclusively to go offscreen 
DCs/surfaces (I'm not sure the terminology since Window-Eyes deals with 
surfaces but I think user land code uses HDCs.)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-15 14:09:25 PDT
(In reply to comment #8)
> The problematic textouts seem almost exclusively to go offscreen 
> DCs/surfaces (I'm not sure the terminology since Window-Eyes deals with 
> surfaces but I think user land code uses HDCs.)

That's because we use double-buffering, so almost all drawing is done to an
offscreen surface.

It sounds like the problem with the ExtTextOut calls is just that we draw
justified text one word at a time in separate GDI calls. I don't know where the
DC stuff is coming from. Aaron, you might want to find out by setting a
breakpoint on DeleteDC with a count of, say, 1000, to get some sample stack traces.
Comment 10 William Smith 2005-09-16 06:47:41 PDT
> It sounds like the problem with the ExtTextOut calls is just that we draw
> justified text one word at a time in separate GDI calls. I don't know where the
> DC stuff is coming from. 

I just checked.    Normally text is plotted one group at a time, but the
justified text is plotted one character at a time.    (For example, on google
one TextOut is "Advanced search"), but the  visuaide page only plots one
character at a time.
Comment 11 Aaron Leventhal 2005-09-16 13:30:32 PDT
Putting any of these rules on <p> for the page makes the screen readers
extremely slow. They all paint character by character.

	text-align:justify;
	font-variant: small-caps;
	letter-spacing: 1px;

Unfortunately, moving the focus is causing the entire page to repaint, thus each
character 1 at a time.

Anyone have a suggested fix? Do we just disable those styles when a screen
reader is running on win32?
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-09-16 13:36:34 PDT
+ word-spacing: 1px;
Comment 13 Aaron Leventhal 2005-09-16 13:55:00 PDT
Yes, word-spacing causes the problem as well.
Comment 14 Aaron Leventhal 2005-09-16 13:55:51 PDT
Roc, why is so much of the page repainting just for a focus move anyway?
Comment 15 Aaron Leventhal 2005-09-16 14:02:00 PDT
Created attachment 196366 [details] [diff] [review]
Never use PaintTextSlowly when a win32 screen reader is present
Comment 16 Aaron Leventhal 2005-09-16 14:08:24 PDT
Roc, is the real issue that we're redrawing so much of the page each time the
outline style changes on a link?

There is no issue in IE with screen reader sluggishness with the testcase
http://www.visuaide.com/HW/en/index.asp

Either they have a more efficient way to paint these styles or they aren't
repainting as much of the page when focus moves. Either way we're missing some
optimizations in Gecko, it seems.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 19:54:32 PDT
(In reply to comment #14)
> Roc, why is so much of the page repainting just for a focus move anyway?

If the focus starts at the frame, then we repaint the entire frame because the
focus ring covers the inside of the frame edge.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 19:57:55 PDT
(In reply to comment #16)
> Roc, is the real issue that we're redrawing so much of the page each time the
> outline style changes on a link?
> 
> There is no issue in IE with screen reader sluggishness with the testcase
> http://www.visuaide.com/HW/en/index.asp
> 
> Either they have a more efficient way to paint these styles or they aren't
> repainting as much of the page when focus moves. Either way we're missing some
> optimizations in Gecko, it seems.

They're probably calling ExtTextOut to pass an array of spacing values. We're
painting one character at a time, indeed. This needs to be fixed in
nsFontMetricsWin::DrawString. It actually shouldn't be that hard.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 20:04:24 PDT
Comment on attachment 196366 [details] [diff] [review]
Never use PaintTextSlowly when a win32 screen reader is present

This could have all kinds of consequences ... on-screen garbage etc. Let's not
do this unless we absolutely have to.
Comment 20 Aaron Leventhal 2005-09-18 18:26:59 PDT
(In reply to comment #19)
> (From update of attachment 196366 [details] [diff] [review] [edit])
> This could have all kinds of consequences ... on-screen garbage etc. Let's not
> do this unless we absolutely have to.

I have a test build that contains this patch up here:
ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/accessibility/firefox/firefox-9-16-2005-branch-1.5-win32.exe

The screen redraws to removed/use thses styles as soon as the screen reader is
loaded/unloaded. The only bad thing I have seen so far is underlines that are
too long. What is a case with one of these 4 rarely used styles where garbage
may be visible when there is a screen reader?

Robert, do you have any idea why the entire screen is redrawn in that page each
time focus moves? What's the 'right' way to fix this? Make things as optimal as
they are in IE for this page? Is that something that is likely to happen on
branch? Since you're minusing the bug, can you recommend another way?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-18 18:54:30 PDT
(In reply to comment #20)
> The screen redraws to removed/use thses styles as soon as the screen reader is
> loaded/unloaded. The only bad thing I have seen so far is underlines that are
> too long.

How about selection?
How about lines with different font sizes/styles on the same line?
How about negative letter-spacing?

The problem is that we're reflowing frames assuming that the justification
space/letter spacing is correct, but we're painting them without that spacing.
That's not SO bad if the spacing increases the frame size ... it just looks bad
and may make event targeting like selection work poorly ... but it's terrible if
the spacing decreases the frame size, because then we repaint outside the frame
bounds.

> Robert, do you have any idea why the entire screen is redrawn in that page
> each time focus moves? What's the 'right' way to fix this? Make things as
> optimal as they are in IE for this page? Is that something that is likely to
> happen on branch? Since you're minusing the bug, can you recommend another
> way?

File a bug with a simplified test case. I'm still not sure exactly which tabbing
scenario you're talking about. If you tab either to or from the document frame
itself, then it's comment #17.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-18 18:56:36 PDT
If you're just tabbing from one link in the page to another link, then you
shouldn't see a full-page paint, so file a bug. If it's something simple at root
then maybe we can get a branch fix.

Using ExtTextOut on Windows (comment #18) may also be a potential branch fix.
Comment 23 Aaron Leventhal 2005-09-18 19:58:13 PDT
(In reply to comment #22)
> If you're just tabbing from one link in the page to another link, then you
> shouldn't see a full-page paint, so file a bug. If it's something simple at root
> then maybe we can get a branch fix.
Yes, that's what I'm seeing. I filed bug 309099 for it.

> Using ExtTextOut on Windows (comment #18) may also be a potential branch fix.
That sounds like the right thing to do to, so I filed bug 309101 for it, but I
think the crucial one is bug 309099.
Comment 24 Mike Schroepfer 2005-09-19 15:58:39 PDT
Minusing for 1.8b5 since this is in core code and we don't yet have a safe patch.
Comment 25 Simon Fraser 2005-09-22 08:29:45 PDT
Isn't a better approach here to move the rendering of text with word- and
letter-spacing into GFX (so, if it can, make the OS do the rendering of spaced
and justified text)? This relates to bug 157967.
Comment 26 Aaron Leventhal 2005-09-22 08:37:02 PDT
(In reply to comment #25)
> Isn't a better approach here to move the rendering of text with word- and
> letter-spacing into GFX (so, if it can, make the OS do the rendering of spaced
> and justified text)? This relates to bug 157967.

Simon, when you say better, what are you comparing to?
Are you talking about the patch that was minused?
Our our current approach to fix this via bug 309099 and bug 279074.

If the latter, is bug 157967 an alternative to fixing bug 279074? We definitely 
need to prevent the full screen from repainting on focus moves.
Comment 27 Simon Fraser 2005-09-22 08:43:17 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > Isn't a better approach here to move the rendering of text with word- and
> > letter-spacing into GFX (so, if it can, make the OS do the rendering of spaced
> > and justified text)? This relates to bug 157967.
> 
> Simon, when you say better, what are you comparing to?

Better in the sense that we should avoid drawing text word by word, or character
by character if word or letter spacing is in effect (per comment #9).
Comment 28 Aaron Leventhal 2005-09-22 09:13:50 PDT
(In reply to comment #27)
> Better in the sense that we should avoid drawing text word by word, or 
character ...

Right, that's bug 279074. If the other one you mentioned is better I'm sure Roc 
is on top of that.
Comment 29 Jungshik Shin 2005-09-23 10:43:48 PDT
(In reply to comment #18)

> They're probably calling ExtTextOut to pass an array of spacing values.

I guess they don't call ExtTextOut directly as we do. By relying on Uniscribe
APIs instead, in effect, I believe they do what you thought they do. That's what
sfraser was talking about, I think, in comment #25 (and bug 157967)
Comment 30 Aaron Leventhal 2005-09-23 12:40:08 PDT
The patch to use ExtTextOut in bug 297074 fixes the lion's share of the problem.
Should we try to take that large-ish fix for branch or look for an alternative
such as fixing bug 309099?
Comment 31 Aaron Leventhal 2005-09-30 18:00:17 PDT
Fixed by checkin to bug 309099. The fix in bug 297074 would improve things
further but isn't as critical.
Comment 32 Aaron Leventhal 2005-09-30 21:40:15 PDT
Fixed well enough via checkin for bug 309099. In version 2.0 it will be even
better once we use better text output methods.

Note You need to log in before you can comment on or make changes to this bug.