msaa/nsTextAccessibleWrap::get_fontFamily probably doesn't do what it's supposed to

RESOLVED FIXED in mozilla5

Status

()

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

People

(Reporter: zwol, Assigned: zwol)

Tracking

Trunk
mozilla5
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
msaa's implementation of nsTextAccessibleWrap::get_fontFamily is using a gfx API (nsIDeviceContext::FirstExistingFont) that hasn't done anything particularly useful in some time (it doesn't bother to check whether the font really exists, so you just get the first font in the font stack from the style system).  This is the last use of this API in the tree.  There are better alternatives, but to fix it properly, I need to know what this function is *supposed* to do.  It appears to me that it's being called by an accessibility program external to Firefox, so we can't just delete it, but I have no idea what those programs expect.  There is no analogous function in the Mac or ATK accessibility back ends.

Incidentally, ::GetCharacterExtents in the same file is *also* probably not doing what it's supposed to do, although for entirely different reasons.

Comment 1

7 years ago
ISimpleDOMText is old interface, though maintained. This API has restriction and we can't implement it correctly. The fontFamily method problem is this method is called for whole text node but the node can have several font-families used. Perhaps it should return the first used font-family if we can't detect the "primary" font-family.

The similar problem exists in nsHyperTextAccessible::GetTextAttributes in font-family case, though it doesn't have restrictions as get_fontFamily has. There we can do correct things I think since it returns range within the node where font-family is constant.

Can you suggest API that we should rely on?

And could you file a bug for GetCharacterExtents problem?
(Assignee)

Comment 2

7 years ago
I'm not sure exactly how to do it, but I'm pretty sure there *is* a better way.  cc:ing the font guys.

I don't see any font-related code in nsHyperTextAccessible::GetTextAttributes...?

Filed bug 648282 on GetCharacterExtents.
(Assignee)

Comment 3

7 years ago
Poking around a little more, I think code similar to what's in nsFontWeightTextAttr::GetFontWeight should do the job -- although I'm not sure why that function is doing lookups into gfx while all the other nsFont*TextAttr getters are content to believe what the style system tells them.

Comment 4

7 years ago
(In reply to comment #2)
> I'm not sure exactly how to do it, but I'm pretty sure there *is* a better way.
>  cc:ing the font guys.
> 
> I don't see any font-related code in
> nsHyperTextAccessible::GetTextAttributes...?

It's based on CSS font-family what's wrong (nsTextAttrs.cpp).
(Assignee)

Comment 5

7 years ago
Please tell me what all this stuff is SUPPOSED to do.  Specifically: my immediate inclination upon reading nsTextAttrs.cpp is to make it rely *more* on nsCSSTextAttr -- rip out the special cases for font-size, font-weight, background color -- rather than *less* -- don't bother calling down to gfx at all, just take everything from the style system -- but you're telling me this is wrong.  Okay.  What's *right*?  If implementation limits were completely irrelevant, what would the externally visible semantics of these functions be?
(Assignee)

Comment 6

7 years ago
Created attachment 524512 [details] [diff] [review]
patch

I think this is probably the right change, at least short term.  Note: not yet even compiled (will push to try shortly), and I have no way of testing anything MSAA-related.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
Attachment #524512 - Flags: review?(surkov.alexander)

Comment 7

7 years ago
(In reply to comment #6)
> Created attachment 524512 [details] [diff] [review]
> patch
> 
> I think this is probably the right change, at least short term.  Note: not yet
> even compiled (will push to try shortly), and I have no way of testing anything
> MSAA-related.

I don't have any tools that allow to test ISimpleDOM interfaces, though I could send a build to developers who use these interfaces to check if everything is ok. Perhaps Marco is aware how to test this method though.

Comment 8

7 years ago
Comment on attachment 524512 [details] [diff] [review]
patch


>+  const nsStyleFont *styleFont = frame->GetStyleFont();

nit: please do const nsStyleFont* styleFont, instead const nsStyleFont *styleFont
Attachment #524512 - Flags: review?(surkov.alexander) → review+

Comment 9

7 years ago
(In reply to comment #5)
> Please tell me what all this stuff is SUPPOSED to do.  Specifically: my
> immediate inclination upon reading nsTextAttrs.cpp is to make it rely *more* on
> nsCSSTextAttr -- rip out the special cases for font-size, font-weight,
> background color -- rather than *less* -- don't bother calling down to gfx at
> all, just take everything from the style system -- but you're telling me this
> is wrong.

We used style system to get text attributes as a first implementation of this method. When we realized the style system doesn't provide us what we need then we started to introduce specific classes.

>  Okay.  What's *right*?  If implementation limits were completely
> irrelevant, what would the externally visible semantics of these functions be?

If I get the following scenario is possible:
<p>FontFamily1FontFamily2</p> so there's single text node and two font families are used for it.

When you request text attributes at 0 offset then we should return a range [0, 11) (for string FontFamily1), when you request them at 11 offset then we should return a range [11, 22) (for string FontFamily2). If text attributes computation is CSS based then we can get font family for whole text node only and will return [0, 22) range, that's incorrect.
(Assignee)

Comment 10

7 years ago
(In reply to comment #8)
> Comment on attachment 524512 [details] [diff] [review]
> patch
> 
> 
> >+  const nsStyleFont *styleFont = frame->GetStyleFont();
> 
> nit: please do const nsStyleFont* styleFont, instead const nsStyleFont
> *styleFont

OK, but as a matter of fact that variable is unused, so instead I just deleted the line.  I did fix the position of the star on another variable in the same function, though.
(Assignee)

Comment 11

7 years ago
http://hg.mozilla.org/mozilla-central/rev/f7d8fe104567
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

7 years ago
(In reply to comment #9)
> If I get the following scenario is possible:
> <p>FontFamily1FontFamily2</p> so there's single text node and two font families
> are used for it.
> 
> When you request text attributes at 0 offset then we should return a range [0,
> 11) (for string FontFamily1), when you request them at 11 offset then we should
> return a range [11, 22) (for string FontFamily2). If text attributes
> computation is CSS based then we can get font family for whole text node only
> and will return [0, 22) range, that's incorrect.

Ok, I get it now.  I don't know if the right hooks exist to do this, though -- you'd need information that may be being thrown away after painting.  nsTextFrameThebes is largely a mystery to me.

Updated

7 years ago
OS: Linux → Windows 7
Target Milestone: --- → mozilla2.2
Version: unspecified → Trunk
(Assignee)

Updated

7 years ago
Blocks: 648197
You need to log in before you can comment on or make changes to this bug.