Last Comment Bug 473570 - Restrict font-weight text attribute to allowed values
: Restrict font-weight text attribute to allowed values
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on: 467669
Blocks: textattra11y
  Show dependency treegraph
 
Reported: 2009-01-14 08:19 PST by Aaron Leventhal
Modified: 2009-03-03 22:50 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mochitest (2.95 KB, patch)
2009-02-16 05:21 PST, alexander :surkov
no flags Details | Diff | Review
simple testcase (840 bytes, text/html)
2009-02-16 20:17 PST, John Daggett (:jtd)
no flags Details
patch (12.04 KB, patch)
2009-02-17 03:54 PST, alexander :surkov
mzehe: review+
roc: superreview+
Details | Diff | Review
failures on linux (119.92 KB, text/plain)
2009-02-17 09:47 PST, David Bolter [:davidb]
no flags Details
patch2 (21.16 KB, patch)
2009-03-03 03:09 PST, alexander :surkov
dbolter: review+
Details | Diff | Review
patch3 (21.41 KB, patch)
2009-03-03 19:00 PST, alexander :surkov
karlt: review+
Details | Diff | Review

Description Aaron Leventhal 2009-01-14 08:19:08 PST
font-weight can only be 100, 200, 300, 400, 500, 600, 700, 800, 900

normal = 400, bold = 700 

Right now we will expose whatever the author provides, including bolder, lighter, etc.
Comment 1 David Bolter [:davidb] 2009-01-14 12:40:06 PST
I'll try using surkovs handy gCSSTextAttrsMap
Comment 2 David Bolter [:davidb] 2009-01-14 13:02:07 PST
Hmmm... well that didn't work.
Comment 3 John Daggett (:jtd) 2009-02-15 22:32:30 PST
What's the testcase to reproduce the problem this bug is meant to describe?  Aaron, could you attach a simple testcase?
Comment 4 alexander :surkov 2009-02-16 05:21:45 PST
Created attachment 362568 [details] [diff] [review]
mochitest

(In reply to comment #0)
> font-weight can only be 100, 200, 300, 400, 500, 600, 700, 800, 900

> Right now we will expose whatever the author provides, including bolder,
> lighter, etc.

Actually, we don't expose whatever author specified like "lighter" or "bolder" but any way it sounds we don't follow CSS spec requirements (http://www.w3.org/TR/CSS2/fonts.html#propdef-font-weight), we expose values like "401", "402". 

(In reply to comment #3)
> What's the testcase to reproduce the problem this bug is meant to describe? 

I guess attached mochitest is enough.
Comment 5 Marco Zehe (:MarcoZ) 2009-02-16 05:27:25 PST
Comment on attachment 362568 [details] [diff] [review]
mochitest

Agreed, this should be OK to cover this case.
Comment 6 alexander :surkov 2009-02-16 19:44:36 PST
cc'ing additional people who might know.

<p id="area11" style="font-weight: bolder;">
    <span style="font-weight: bolder;">bolder</span>bolder
    <span style="font-weight: lighter;">lighter</span>bolder
    <span style="font-weight: normal;">normal</span>bolder
    <b>bold</b>bolder
    <span style="font-weight: 400;">normal</span>bolder
    <span style="font-weight: bold;">bold</span>bolder
    <span style="font-weight: 700;">bold</span>bolder
  </p>

html:p - "401"
1st html:span - "402"
2nd html:span - "401"
3d html:span - "400"
html:b - "402"
4d html:span - "bold"
5th html:span - "bold"

CSS spec doesn't list "401", "402" (http://www.w3.org/TR/CSS2/fonts.html#propdef-font-weight) in allowed values. Is it a bug these numbers are exposed in computed styles?

Btw, html:b means bolder, not bold?
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-16 19:48:48 PST
> Is it a bug these numbers are exposed in computed styles?

Yes.  See the XXX comments in nsComputedDOMStyle::GetFontWeight.  Pretty sure it's filed, too.
Comment 8 John Daggett (:jtd) 2009-02-16 20:16:29 PST
The strange values are "relative" degrees of bolder/lighter, so 402 means 400 plus two steps bolder.  There are unresolved spec issues with exactly how bolder/lighter are calculated and what getComputedValue should return here.  The actual weight used is determined within gfx code, based on the available faces for a given font.  With font fallback, this can end up varying within a given run of text for which the same style is used.

I'm happy to resolve this as 402 == 400 + (2 * 100) or some other calculation that makes assumptions.  Though not "correct" it certainly would be less wrong than what is returned by getComputedStyle today.
Comment 9 John Daggett (:jtd) 2009-02-16 20:17:57 PST
Created attachment 362673 [details]
simple testcase

Computed weight with bolder set returned as 401.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-16 20:24:36 PST
There have also been proposals floated around for allowing the font-weight syntax to specify something like "500 bolder" (primarily because we've seen cases where a computed value can't actually be re-specified to be a bug in CSS because of the effects on scripts, editing tools, etc., and not because anybody actually wants that value).  That probably depends on figuring out exactly how bolder works, though.  We *could* implement some proposed syntax for that and return it from getComputedStyle.

Another (hopefully temporary) option is just returning "bolder" and "lighter" from getComputedStyle, which is like what I did for font-stretch.

I'm not sure whether or not a bug was filed.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-16 20:33:46 PST
See bug 77882?
Comment 12 alexander :surkov 2009-02-16 21:07:24 PST
From a11y point of view we don't want neither 402 nor bolder nor 500 (if there proper font is not available). If visually 402 or 500 are the same like 700 then I believe we need to expose 700. Sure, getComputedStyle is for developers and they might want to have "500" or "bolder".
Comment 13 alexander :surkov 2009-02-17 03:54:38 PST
Created attachment 362699 [details] [diff] [review]
patch
Comment 14 alexander :surkov 2009-02-17 04:54:12 PST
getting back previous bug summary because it's a11y bug and that summary reflects better what we need to fix here.

John, I would guess you could use 77882 to implement your proposition in comment #8 (or David's in comment #10). Any way getComputedStyle problem sounds like a bit different issue than we need to fix here. But if getComputedStyle behaviour will match to behaviour we need for accessibility then we could consider to get back to it.
Comment 15 Marco Zehe (:MarcoZ) 2009-02-17 05:14:02 PST
Comment on attachment 362699 [details] [diff] [review]
patch

Alex, I don't see a test for the "900" font weight here. Is that not exposed as anything higher than 700?
Comment 16 alexander :surkov 2009-02-17 05:30:30 PST
(In reply to comment #15)
> (From update of attachment 362699 [details] [diff] [review])
> Alex, I don't see a test for the "900" font weight here. Is that not exposed as
> anything higher than 700?

Sounds so, at least for default font on Windows. I hope David Bolter will try on Linux. I will test on Mac.
Comment 17 Marco Zehe (:MarcoZ) 2009-02-17 05:52:06 PST
Comment on attachment 362699 [details] [diff] [review]
patch

OK, r=me with that possibly fixed if platform specific stuff turns up.
Comment 18 David Bolter [:davidb] 2009-02-17 09:47:49 PST
Created attachment 362734 [details]
failures on linux

Surkov, I can confirm tests pass on Mac, but unfortunately there is borkage on Linux. I attach the relevant mochitest log here.
Comment 19 alexander :surkov 2009-02-17 19:26:50 PST
(In reply to comment #18)
> Created an attachment (id=362734) [details]
> failures on linux
> 
> Surkov, I can confirm tests pass on Mac, but unfortunately there is borkage on
> Linux. I attach the relevant mochitest log here.

It sounds your font on Linux doesn't make difference between normal and bold text because it always return "500" value. Is it true? Or something wrong with my code?
Comment 20 David Bolter [:davidb] 2009-02-18 12:20:33 PST
(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=362734) [details] [details]
> > failures on linux
> > 
> > Surkov, I can confirm tests pass on Mac, but unfortunately there is borkage on
> > Linux. I attach the relevant mochitest log here.
> 
> It sounds your font on Linux doesn't make difference between normal and bold
> text because it always return "500" value. Is it true? Or something wrong with
> my code?

Yes it seems that is what is happening.
Comment 21 alexander :surkov 2009-02-19 07:05:00 PST
Btw, Zoom doesn't affect on font size and font weight.
Comment 22 alexander :surkov 2009-02-20 04:49:57 PST
Roc, why do these examples produce "500" font-weight on linux? But visually "Bold" is bold, "Normal" is normal.

<b id="area0" role="section">Bold</b>
<p id="area00" role="section">Normal</p>

Does gfx stuffs used in the patch work well on linux?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-20 12:11:15 PST
I don't know.
Comment 24 Karl Tomlinson (ni?:karlt) 2009-02-22 19:20:48 PST
Comment on attachment 362699 [details] [diff] [review]
patch

>+PRInt32
>+nsFontWeightTextAttr::GetFontWeight(nsIFrame *aFrame)
>+{
>+  nsStyleFont* styleFont =
>+    (nsStyleFont*)(aFrame->GetStyleDataExternal(eStyleStruct_Font));
>+
>+  gfxUserFontSet *fs = aFrame->PresContext()->GetUserFontSet();
>+
>+  nsCOMPtr<nsIFontMetrics> fm;
>+  aFrame->PresContext()->DeviceContext()->
>+    GetMetricsFor(styleFont->mFont, aFrame->GetStyleVisibility()->mLangGroup,
>+                  fs, *getter_AddRefs(fm));
>+
>+  nsCOMPtr<nsIThebesFontMetrics> tfm = do_QueryInterface(fm);
>+  gfxFontGroup *fontGroup = tfm->GetThebesFontGroup();
>+  gfxFont *font = fontGroup->GetFontAt(0);
>+  gfxFontEntry *fontEntry = font->GetFontEntry();
>+
>+  return fontEntry->Weight();
>+}

The fontEntry is used for different things on different platforms.
The Linux code uses the gfxFontEntry(const nsAString& aName) constructor which doesn't initialize the weight field.

On Linux, "font->GetStyle()->weight" will give the absolute weight requested of the font face.

On Windows, "font->GetStyle()->weight" will give the same weight as "fontEntry->Weight()", the weight of the first font in the font group, which may not be the weight of the font face used to render the characters.

On Mac, "font->GetStyle()->weight" will just give the same number as getComputedStyle(), as described in comment 8.  fontEntry->Weight() will give the weight of the font face used but won't look like a bold weight when synthetic bolding is used (when there is no bold font in the family).

But I would have thought that the appropriate number for the text attribute should not depend on the set of fonts installed?  Shouldn't it instead give an indication of what the author requests, so that the client can present this in the best way it knows?

(In reply to comment #0)
> Right now we will expose whatever the author provides, including bolder,
> lighter, etc.

What is the spec that says that bolder/lighter are not allowed?

Given that bolder/lighter are not allowed, wouldn't it be best to apply adjustments to the base absolute weight based on the relative number of bolder/lighter steps (something like suggested in comment 8).
If clients of the text attributes are not likely to resolve weights to the precision of units of 100, then perhaps use larger steps.

>diff --git a/accessible/tests/mochitest/test_textattrs.html b/accessible/tests/mochitest/test_textattrs.html

It looks like the mochitest just expects bolder/lighter to select between fonts of weight 400 or 700, irrespective of what font faces are available.  If that is what is wanted, wouldn't it be best to just provide that?
Comment 25 alexander :surkov 2009-02-23 19:01:58 PST
(In reply to comment #24)

> But I would have thought that the appropriate number for the text attribute
> should not depend on the set of fonts installed?  Shouldn't it instead give an
> indication of what the author requests, so that the client can present this in
> the best way it knows?

I am of opinion blind users should "see" the same sighted users see. If we give AT anything web page author wanted to provide then AT will be able to decide what to do with this. But I'm not sure that's the way we should follow. Spec doesn't address this.
 
> (In reply to comment #0)
> > Right now we will expose whatever the author provides, including bolder,
> > lighter, etc.
> 
> What is the spec that says that bolder/lighter are not allowed?

http://www.linuxfoundation.org/en/Accessibility/IAccessible2/TextAttributes

> It looks like the mochitest just expects bolder/lighter to select between fonts
> of weight 400 or 700, irrespective of what font faces are available.  If that
> is what is wanted, wouldn't it be best to just provide that?

It's not I wanted but do I have the better way? Do I have an access to information about installed fonts from script?
Comment 26 Karl Tomlinson (ni?:karlt) 2009-02-23 19:40:27 PST
(In reply to comment #25)
> > It looks like the mochitest just expects bolder/lighter to select between fonts
> > of weight 400 or 700, irrespective of what font faces are available.  If that
> > is what is wanted, wouldn't it be best to just provide that?
> 
> It's not I wanted but do I have the better way? Do I have an access to
> information about installed fonts from script?

Ideally we want tests to only test the specific desired functionality, but in practice tests unfortunately end up testing other assumptions also.

Currently, the only info available from script is a list of families through:

http://mxr.mozilla.org/mozilla-central/source/gfx/idl/nsIFontEnumerator.idl

This is used by:

http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug394057.html

It would be possible to detect which families are installed, have a table of weights likely to be available for some set of known families and test based on that table, but that's probably more complicated than you want, and still not assumption free.

There are things here that can be tested without making assumptions though:

* The attribute must be an allowed value.

* With CSS font-weight x > y (and all other factors equal), the text attribute
  font-weight f(x) >= f(y).

* For an element a with CSS font-weight bolder, and parent element b, the text
  attribute font-weight g(a) >= g(b).

There are even some cases where ">=" can be strictly ">" because all platforms have synthetic bold, but that requires getting synthetic bold into the text attribute.
Comment 27 alexander :surkov 2009-02-23 20:57:20 PST
I like f(x) >= f(y) approach, but it's possibly it makes sense to compare text attribute values between each other.

As well I filed letter to IA2 mail list to clarify how font-weight should be computed.
Comment 28 alexander :surkov 2009-03-02 22:03:20 PST
Ok, I got confirmation from IA2 group, Pete said:

"What is important is what the end user sees.  It sounds like the FF user will only see normal and bold so that's what FF should expose."
Comment 29 alexander :surkov 2009-03-03 03:09:19 PST
Created attachment 365162 [details] [diff] [review]
patch2

Karl, could you review layout stuffs usage, does it corresponds to the rule "what end user sees is what screen readers get"?
Comment 30 David Bolter [:davidb] 2009-03-03 06:52:51 PST
Comment on attachment 365162 [details] [diff] [review]
patch2

>+    function equalsToNormal(aWeight)
>+    {
>+      return aWeight <= 400;
>+    }
>+
>+    function equalsToBold(aWeight)
>+    {
>+      return aWeight > 400;
>+    }

Nit: how about find and replace "equalsTo" with "is"?

r=me either way.
Comment 31 Karl Tomlinson (ni?:karlt) 2009-03-03 18:28:33 PST
Comment on attachment 365162 [details] [diff] [review]
patch2

+#ifdef MOZ_ACCESSIBILITY_ATK
+  return font->GetStyle()->weight;
+#endif
+
+  gfxFontEntry *fontEntry = font->GetFontEntry();
+  return fontEntry->Weight();

Use "#else" and move "#endif" so that unreachable gfxFontEntry code is not
compiled, which may result in compiler warnings.

Testing MOZ_PANGO would be better than MOZ_ACCESSIBILITY_ATK, as it is the
features of the font backend rather than ATK that are relevant here.
It is possible to configure Mozilla with ATK but with an FT2 backend which
would need the gfxFontEntry code.

> Karl, could you review layout stuffs usage, does it corresponds to the rule
> "what end user sees is what screen readers get"?

I think this code should give a good match in most situations.

The one situation where I can see it not giving an appropriate number is on
the Mac platform when there doesn't exist a bold font in the family and so the
rendering of a non-bold font face is changed so that the user sees what looks
like a bold font.  Currently this code would return the weight on the non-bold
face, but I think the weight text attribute should be a "bold" weight.
Just using the number 700 for a bold weight should be fine.

This situation can be detected with gfxFont::IsSyntheticBold().  This function
is only needed on Mac, but it is "safe" to use on all platforms.  (For non-Mac
platforms it always return false.)

A simple

  if (font->IsSyntheticBold())
      return 700;

before the gfxFontEntry code should work fine (given our current
implementation at least).
Comment 32 Karl Tomlinson (ni?:karlt) 2009-03-03 18:36:54 PST
BTW, this would all be easier if we had bug 467669
Comment 33 alexander :surkov 2009-03-03 19:00:30 PST
Created attachment 365366 [details] [diff] [review]
patch3

Karl, I addressed your comments and added some your wording into code - I believe this should make code more readable because technically a11y programmers aren't familiar with gfx code :)
Comment 34 Karl Tomlinson (ni?:karlt) 2009-03-03 19:08:57 PST
Comment on attachment 365366 [details] [diff] [review]
patch3

Looks good, thanks.
Comment 35 alexander :surkov 2009-03-03 22:48:01 PST
checked in http://hg.mozilla.org/mozilla-central/rev/9be036855586

thanks for reviews!
Comment 36 alexander :surkov 2009-03-03 22:50:23 PST
(In reply to comment #30)

> Nit: how about find and replace "equalsTo" with "is"?
> 
> r=me either way.

"is" sounds good for me as well but eventually I didn't address this.

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