The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, fixed1.8})

Trunk
x86
Windows XP
access, fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago

Comment 1

12 years ago
Does the problem occur when the user turns off stylesheets on the page? (View, 
Page Style, No Style)
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Comment 6

12 years ago
> 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?

Comment 8

12 years ago
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.

Comment 10

12 years ago
> 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.
(Assignee)

Comment 11

12 years ago
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
(Assignee)

Comment 13

12 years ago
Yes, word-spacing causes the problem as well.
(Assignee)

Comment 14

12 years ago
Roc, why is so much of the page repainting just for a focus move anyway?
(Assignee)

Comment 15

12 years ago
Created attachment 196366 [details] [diff] [review]
Never use PaintTextSlowly when a win32 screen reader is present
Attachment #196366 - Flags: review?(roc)
(Assignee)

Comment 16

12 years ago
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-
(Assignee)

Comment 20

12 years ago
(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?
(Assignee)

Updated

12 years ago
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.
(Assignee)

Updated

12 years ago
Depends on: 309099
(Assignee)

Comment 23

12 years ago
(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 ]

Comment 24

12 years ago
Minusing for 1.8b5 since this is in core code and we don't yet have a safe patch.
Flags: blocking1.8b5? → blocking1.8b5-

Comment 25

12 years ago
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.
(Assignee)

Comment 26

12 years ago
(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

12 years ago
(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).
(Assignee)

Comment 28

12 years ago
(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

12 years ago
(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)
(Assignee)

Updated

12 years ago
Depends on: 297074
(Assignee)

Comment 30

12 years ago
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) ]
(Assignee)

Comment 31

12 years ago
Fixed by checkin to bug 309099. The fix in bug 297074 would improve things
further but isn't as critical.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

12 years ago
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.