Closed Bug 473570 Opened 15 years ago Closed 15 years ago

Restrict font-weight text attribute to allowed values

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files, 2 obsolete files)

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.
I'll try using surkovs handy gCSSTextAttrsMap
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED
Hmmm... well that didn't work.
Assignee: david.bolter → nobody
Status: ASSIGNED → NEW
What's the testcase to reproduce the problem this bug is meant to describe?  Aaron, could you attach a simple testcase?
Attached patch mochitestSplinter Review
(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 on attachment 362568 [details] [diff] [review]
mochitest

Agreed, this should be OK to cover this case.
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?
> 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.
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.
Assignee: nobody → jdaggett
Summary: Restrict font-weight to allowed values → getComputedValue returns relative weight values when bolder/lighter used
Attached file simple testcase
Computed weight with bolder set returned as 401.
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.
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".
Attached patch patch (obsolete) — Splinter Review
Assignee: jdaggett → surkov.alexander
Status: NEW → ASSIGNED
Attachment #362699 - Flags: superreview?(roc)
Attachment #362699 - Flags: review?(marco.zehe)
Attachment #362699 - Flags: review?(david.bolter)
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.
Summary: getComputedValue returns relative weight values when bolder/lighter used → Restrict font-weight text attribute to allowed values
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?
(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 on attachment 362699 [details] [diff] [review]
patch

OK, r=me with that possibly fixed if platform specific stuff turns up.
Attachment #362699 - Flags: review?(marco.zehe) → review+
Attached file 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.
(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?
Attachment #362699 - Flags: superreview?(roc) → superreview+
(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.
Btw, Zoom doesn't affect on font size and font weight.
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 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?
(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?
(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.
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.
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."
Attached patch patch2 (obsolete) — Splinter Review
Karl, could you review layout stuffs usage, does it corresponds to the rule "what end user sees is what screen readers get"?
Attachment #362699 - Attachment is obsolete: true
Attachment #365162 - Flags: review?(mozbugz)
Attachment #365162 - Flags: review?(david.bolter)
OS: Windows XP → All
Hardware: x86 → All
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.
Attachment #365162 - Flags: review?(david.bolter) → review+
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).
BTW, this would all be easier if we had bug 467669
Depends on: 467669
Attached patch patch3Splinter Review
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 :)
Attachment #365162 - Attachment is obsolete: true
Attachment #365366 - Flags: review?(mozbugz)
Attachment #365162 - Flags: review?(mozbugz)
Attachment #365366 - Flags: review?(mozbugz) → review+
Comment on attachment 365366 [details] [diff] [review]
patch3

Looks good, thanks.
checked in http://hg.mozilla.org/mozilla-central/rev/9be036855586

thanks for reviews!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: