Closed Bug 307545 Opened 19 years ago Closed 19 years ago

4 ways to use CSS to make win32-based screen readers crawl

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

(
URL
)

Details

(Keywords: access, fixed1.8, Whiteboard: [ETA: let's fix this the right way in bug 297074 (has patch) or bug 309099 (assigned to roc) ])

Attachments

(1 file)

Never use PaintTextSlowly when a win32 screen reader is present
2.21 KB, patch
roc
: review-
Details | Diff | Splinter Review 
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.
Does the problem occur when the user turns off stylesheets on the page? (View, 
Page Style, No Style)
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.

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?
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.
(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.
> We draw one rectangle for each dot in the outline.

Yes, but he was talking about text draws.

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?
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.)
(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.
> 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.
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?
Flags: blocking1.8b5?
Summary: CSS text-align: justify makes win32-based screen readers slow → 3 ways to use CSS to make win32-based screen readers crawl
+ word-spacing: 1px;
Summary: 3 ways to use CSS to make win32-based screen readers crawl → 4 ways to use CSS to make win32-based screen readers crawl
Yes, word-spacing causes the problem as well.
Roc, why is so much of the page repainting just for a focus move anyway?
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.
(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.
(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 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.
Attachment #196366 - Flags: review?(roc) → review-
(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?
Whiteboard: [ETA: Roc wants to fix this the right way, but I would need help for that. I suggest a layout guru do it or we consider accepting my patch which only affects page layout when a screen reader is running]
(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.
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.
Depends on: 309099
(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.
Depends on: 309101
Whiteboard: [ETA: Roc wants to fix this the right way, but I would need help for that. I suggest a layout guru do it or we consider accepting my patch which only affects page layout when a screen reader is running] → [ETA: Roc wants to fix this the right way in bug 309099 ]
Minusing for 1.8b5 since this is in core code and we don't yet have a safe patch.
Flags: blocking1.8b5? → blocking1.8b5-
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.
(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.
(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).

(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.
(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)
Depends on: 297074
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?
Whiteboard: [ETA: Roc wants to fix this the right way in bug 309099 ] → [ETA: let's fix this the right way in bug 297074 (has patch) or bug 309099 (assigned to roc) ]
Fixed by checkin to bug 309099. The fix in bug 297074 would improve things
further but isn't as critical.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Fixed well enough via checkin for bug 309099. In version 2.0 it will be even
better once we use better text output methods.
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: