If the font-family rule uses a comma separated list of preferred fonts, we only want to expose the font that's actually being used. The user won't want to hear a list of 4 fonts. We do this currently for ISimpleDOMText::fontFamily, so the code for that is here: http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsTextAccessibleWrap.cpp#286 It does look a bit long and complex though, we should see if there's a quicker way to get the real font used by the current frame.

The text might use a mixture of fonts, when some characters in the text are not supported by some of the fonts.

Can we find out the main font used? That's what the screen reader user wants to know. The rest is more like an implementation detail, right?

Depends on what you mean by the main font. If you can define that precisely, then I'm sure we can write code to find it :-).

Like, if the author defines Times, Ariel, Helvetica and it uses at least some Times, I'll go with that.

If that is a lot of code then I'd like to try to find a simpler rule. Since the user wants to know what the author's intent was, we could just go with the first font listed if we must.

I sent email to IA2 group for clarification. This patch exposes one font-family if several ones were specified. But I guess we shouldn't expose generic font families like "serif" (this patch does it), instead we should expose really used font-family.

Cc'ing Karl. How can we obtain real font family when generic font family is used (like "monospaced", "serif")? So if for example (possibly wrong example) if "serif" is mapped to "Times" font family then we should return "Times" font family. Would gfxFontEntry::Name() return what we need?

(In reply to comment #7) > How can we obtain real font family when generic font family is used (like > "monospaced", "serif")? So if for example (possibly wrong example) if "serif" > is mapped to "Times" font family then we should return "Times" font family. > Would gfxFontEntry::Name() return what we need? gfxFontEntry::Name() is actually identifies a face, a particular font file. On some platforms, in some situations, this is a family name, but in others it does not. I don't think this is going to be a good solution. I wonder whether looking up the pref families from generics might be a good approximation. This is what happens here: http://hg.mozilla.org/mozilla-central/file/d2b77efac151/gfx/thebes/src/gfxFont.cpp#l988

Note, that the users can define in the preferences which font to use for the generic styles. I think that is what intended to say. Also it shouldn't be too complicated to return the right font, since Firefox already uses the right one for displaying (either a font from the list, for generic fonts the one defined in the preferences or if none of the list is found the default one from the preferences). Can't this function be used for retrieving the right font family calling getComputedStyle()? This is also mentioned in an issue at Firebug: http://code.google.com/p/fbug/issues/detail?id=3052

Perhaps a different bug but may have the same fix. from the report "When I get the text attributes for the "self description" edit field at: http://archive.dojotoolkit.org/nightly/checkout/demos/form/demo.html The font-family comes back as a \ delimited list of font names." dbaron: surkov, font fallback is character-by-character dbaron: surkov, if one font doesn't have a glyph, we look in the next font (In reply to comment #1) > The text might use a mixture of fonts, when some characters in the text are not > supported by some of the fonts. It sounds textAttributes can handle this and return one font, if the font is changed from character to character then it should split text on ranges where the font is persistent.

(In reply to comment #9) > Can't this function be used for > retrieving the right font family calling getComputedStyle()? No, getComputedStyle returns list of font families.

On GTK2 (Linux) systems, isn't this problem made more difficult by the fact that Pango will try to find a glyph in any installed font if the requested font has no glyph for the specified character? There is no guarantee that even the system's serif, sans-serif and monospace fonts will each have a glyph for every valid Unicode codepoint.

Now that bug 467669 has landed, you can use the nsRange::GetUsedFontFaces() API to find out exactly what font(s) we're using for a given DOM range. However, it's not clear to me whether that is always what you'd want, from an a11y perspective. If the author has simply called for "sans-serif", *that* expresses their intent - the fact that certain characters in the text happened to get drawn from Arial because Helvetica didn't include them may be largely irrelevant, and reflecting that in an audio description introduces apparent complexity that the author didn't intend and may never have noticed in the visual rendering.

(In reply to comment #12) > On GTK2 (Linux) systems, isn't this problem made more difficult by the fact > that Pango will try to find a glyph in any installed font if the requested > font has no glyph for the specified character? We do such font fallback on all systems, not just Linux (although the behavior of the Pango + fontconfig version used on Linux may be somewhat different from the implementation we have on Mac & Windows).

This is a problem for NVDA users, particularly now that text formatting is exposed in browse mode. This causes NVDA to report all of the possible font families, which is fairly annoying at best and very confusing at worst.

Comment on attachment 599959 [details] [diff] [review] patch Review of attachment 599959 [details] [diff] [review]: ----------------------------------------------------------------- A couple of drive-by comments, if you'll excuse me poking my nose in here.... ::: accessible/src/base/nsTextAttrs.cpp @@ +489,5 @@ > + nsRefPtr<nsFontMetrics> fm; > + nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm)); > + > + gfxFontGroup* fontGroup = fm->GetThebesFontGroup(); > + gfxFont* font = fontGroup->GetFontAt(0); I don't think this is necessarily what you want. GetFontAt(0) will give you the resolved face for the first item in the font-family list, but that may well _not_ be what the user is actually seeing, depending whether that font actually supports the characters present in the text. It's not all that helpful to tell the user that the text is formatted in Times New Roman if it's actually Japanese that we're rendering with Hiragino Mincho Pro. ::: accessible/tests/mochitest/attributes.js @@ +220,5 @@ > +const kSerifFontFamily = WIN ? "Times New Roman" : > + (LINUX ? "DejaVu Serif" : "MacFont"); > + > +const kCursiveFontFamily = WIN ? "Comic Sans MS" : > + (LINUX ? "DejaVu Serif" : "MacFont"); The hardcoded font names here look like a recipe for test-fragility/non-portability. ::: accessible/tests/mochitest/attributes/test_text.html @@ +541,5 @@ > + > + <p id="area16" style="font-family: sans-serif;"> > + <span style="font-family: monospace;">text</span>text > + <span style="font-family: serif;">text</span>text > + <span style="font-family: Bodoni;">text</span>text Some people are likely to have a font called Bodoni installed; if you want to test for an "absent" font, I'd guess it's better to make up a really unlikely-sounding name such as NotAnAvailableFontFamily.

Comment on attachment 599959 [details] [diff] [review] patch I'm agreeing with Jonathan's comments. (In reply to Jonathan Kew (:jfkthame) from comment #17) > > + nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm)); > > + > > + gfxFontGroup* fontGroup = fm->GetThebesFontGroup(); > > + gfxFont* font = fontGroup->GetFontAt(0); > > I don't think this is necessarily what you want. These options are available: * CSS indicates the authors intent. * The code above indicates the first font requested by the authors that is found on the system. * nsRange::GetUsedFontFaces() indicates which font is used for the characters (and perhaps the layout strut height?) > The hardcoded font names here look like a recipe for > test-fragility/non-portability. Using @font-face in the test with some simple fonts, perhaps copied from layout/reftests/fonts would provide a means to expect some exact matches. For other situations, there are other things that can be tested. e.g. result is not blank, result for sans-serif is not "sans-serif", result for sans-serif differs from result for serif, result for italic is the same as for upright.

(In reply to Jonathan Kew (:jfkthame) from comment #17) > A couple of drive-by comments, if you'll excuse me poking my nose in here.... please do it, extra nose is always welcome :) > ::: accessible/src/base/nsTextAttrs.cpp > @@ +489,5 @@ > > + nsRefPtr<nsFontMetrics> fm; > > + nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm)); > > + > > + gfxFontGroup* fontGroup = fm->GetThebesFontGroup(); > > + gfxFont* font = fontGroup->GetFontAt(0); > > I don't think this is necessarily what you want. GetFontAt(0) will give you > the resolved face for the first item in the font-family list, but that may > well _not_ be what the user is actually seeing, depending whether that font > actually supports the characters present in the text. It's not all that > helpful to tell the user that the text is formatted in Times New Roman if > it's actually Japanese that we're rendering with Hiragino Mincho Pro. that's true but it's certainly improvement over what we have now and it works possibly in 90% of cases. The problem is iteration step is an element node thus to support this we'd need to change that logic. Also I'm not sure about perf hit of doing this. I'd go with follow up instead. > ::: accessible/tests/mochitest/attributes.js > @@ +220,5 @@ > > +const kSerifFontFamily = WIN ? "Times New Roman" : > > + (LINUX ? "DejaVu Serif" : "MacFont"); > > + > > +const kCursiveFontFamily = WIN ? "Comic Sans MS" : > > + (LINUX ? "DejaVu Serif" : "MacFont"); > > The hardcoded font names here look like a recipe for > test-fragility/non-portability. Right that's bad but it seems working (for example we do the same for font size checks which is platform/distributive dependent). I could make it more friednly for porting by using functions instead of hardcoded constants like var kCursiveFontFamily = function(val) { if (WIN) return val == "Comic Sans MS"; return val == "something else"; } Do I have other option? > ::: accessible/tests/mochitest/attributes/test_text.html > @@ +541,5 @@ > > + > > + <p id="area16" style="font-family: sans-serif;"> > > + <span style="font-family: monospace;">text</span>text > > + <span style="font-family: serif;">text</span>text > > + <span style="font-family: Bodoni;">text</span>text > > Some people are likely to have a font called Bodoni installed; if you want > to test for an "absent" font, I'd guess it's better to make up a really > unlikely-sounding name such as NotAnAvailableFontFamily. good point, I'll fix it (after couple years of my original patch I didn't have any idea about Bodoni, I just noticed it doesn't exist on try server platforms).

(In reply to Karl Tomlinson (:karlt) from comment #18) > These options are available: > > * CSS indicates the authors intent. > * The code above indicates the first font requested by the authors that is > found on the system. > * nsRange::GetUsedFontFaces() indicates which font is used for the > characters (and perhaps the layout strut height?) eventually we should end up with 3d option > > The hardcoded font names here look like a recipe for > > test-fragility/non-portability. > > Using @font-face in the test with some simple fonts, perhaps copied from > layout/reftests/fonts would provide a means to expect some exact matches. > > For other situations, there are other things that can be tested. e.g. > result is not blank, result for sans-serif is not "sans-serif", result for > sans-serif differs from result for serif, result for italic is the same as > for upright. this sounds like a good idea

Comment on attachment 599959 [details] [diff] [review] patch > nsTArray<nsITextAttr*> textAttrArray(10); btw why can't we just use a stack allocated C array here? since we know how many things we'll need at build time. >+FontFamilyTextAttr::GetValueFor(nsIContent* aElm, nsAutoString* aValue) >+{ >+ nsIFrame* frame = aElm->GetPrimaryFrame(); >+ if (!frame) >+ return false; can it be null? >+ >+ return GetFontFamily(frame, *aValue); >+} >+ >+void >+FontFamilyTextAttr::Format(const nsAutoString& aValue, >+ nsAString& aFormattedValue) >+{ >+ aFormattedValue = aValue; since nsAutoString is only different in that it has a buffer in the object and this object will be heap allocated I don't think passing nsAutoString as a template arg helps you over nsString, and it may hurt you here by forcing an actual copy of the string instead of transfer of the buffer between strings. >+ gfxFontGroup* fontGroup = fm->GetThebesFontGroup(); >+ gfxFont* font = fontGroup->GetFontAt(0); >+ gfxFontEntry* fontEntry = font->GetFontEntry(); >+ aFamily = fontEntry->FamilyName(); some reason for all these local vars though I don't think it really hurts anything significant >+class FontFamilyTextAttr : public nsTextAttr<nsAutoString> any reason the class declaration can't just be in nsTextAttrs.cpp? >+ /** >+ * Return font family for the given frame. >+ * >+ * @param aFrame [in] the given frame to query font weight >+ * @return font weight copy paste issue? >+const kSerifFontFamily = WIN ? "Times New Roman" : >+ (LINUX ? "DejaVu Serif" : "MacFont"); >+ >+const kCursiveFontFamily = WIN ? "Comic Sans MS" : is mac font a real thing or just temp until we run tests there? >+function fontFamily(aComputedStyle) >+{ >+ var name = aComputedStyle.fontFamily; >+ if (name == "monospace") >+ return kMonospaceFontFamily; >+ if (name == "sans-serif") >+ return kSansSerifFontFamily; >+ if (name == "serif") >+ return kSerifFontFamily; js doesn't have switch? :/

(In reply to Trevor Saunders (:tbsaunde) from comment #21) > Comment on attachment 599959 [details] [diff] [review] > patch > > > nsTArray<nsITextAttr*> textAttrArray(10); > > btw why can't we just use a stack allocated C array here? since we know how > many things we'll need at build time. I think we can but not sure why, shouldn't it be the same? > >+FontFamilyTextAttr::GetValueFor(nsIContent* aElm, nsAutoString* aValue) > >+{ > >+ nsIFrame* frame = aElm->GetPrimaryFrame(); > >+ if (!frame) > >+ return false; > > can it be null? it shouldn't be. I think this is sort of crash protection. > >+ > >+ return GetFontFamily(frame, *aValue); > >+} > >+ > >+void > >+FontFamilyTextAttr::Format(const nsAutoString& aValue, > >+ nsAString& aFormattedValue) > >+{ > >+ aFormattedValue = aValue; > > since nsAutoString is only different in that it has a buffer in the object > and this object will be heap allocated I don't think passing nsAutoString > as a template arg helps you over nsString, and it may hurt you here by > forcing an actual copy of the string instead of transfer of the buffer > between strings. That's right. I'm going to tweak this code in next work. I hope you're ok with that and I don't need to rebase my queue :) > >+ gfxFontGroup* fontGroup = fm->GetThebesFontGroup(); > >+ gfxFont* font = fontGroup->GetFontAt(0); > >+ gfxFontEntry* fontEntry = font->GetFontEntry(); > >+ aFamily = fontEntry->FamilyName(); > > some reason for all these local vars though I don't think it really hurts > anything significant probably it's a little bit easier to detect a crash cause > > >+class FontFamilyTextAttr : public nsTextAttr<nsAutoString> > > any reason the class declaration can't just be in nsTextAttrs.cpp? didn't you were fine in bug 728907 to move all of them under nsTextAttrs class? > >+ /** > >+ * Return font family for the given frame. > >+ * > >+ * @param aFrame [in] the given frame to query font weight > >+ * @return font weight > > copy paste issue? yes > >+const kSerifFontFamily = WIN ? "Times New Roman" : > >+ (LINUX ? "DejaVu Serif" : "MacFont"); > >+ > >+const kCursiveFontFamily = WIN ? "Comic Sans MS" : > > is mac font a real thing or just temp until we run tests there? reworked in last patch > >+function fontFamily(aComputedStyle) > >+{ > >+ var name = aComputedStyle.fontFamily; > >+ if (name == "monospace") > >+ return kMonospaceFontFamily; > >+ if (name == "sans-serif") > >+ return kSansSerifFontFamily; > >+ if (name == "serif") > >+ return kSerifFontFamily; > > js doesn't have switch? :/ I'll change that

(In reply to alexander :surkov from comment #23) > (In reply to Trevor Saunders (:tbsaunde) from comment #21) > > Comment on attachment 599959 [details] [diff] [review] > > patch > > > > > nsTArray<nsITextAttr*> textAttrArray(10); > > > > btw why can't we just use a stack allocated C array here? since we know how > > many things we'll need at build time. > > I think we can but not sure why, shouldn't it be the same? well, nsTArray puts the buffer on the heap, but if you convert it to nsAutoTArray with the appropriate size I think its effectively the same. > That's right. I'm going to tweak this code in next work. I hope you're ok > with that and I don't need to rebase my queue :) ok, fine by me. > didn't you were fine in bug 728907 to move all of them under nsTextAttrs > class? sure, but I made this comment before that discussion.

(In reply to Trevor Saunders (:tbsaunde) from comment #24) > well, nsTArray puts the buffer on the heap, but if you convert it to > nsAutoTArray with the appropriate size I think its effectively the same. maybe a followup? to not tweak a queue > > didn't you were fine in bug 728907 to move all of them under nsTextAttrs > > class? > > sure, but I made this comment before that discussion. yep, I realized that after I commented

I filed bug 732366 for correct font-family text attribute implementation