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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(4 files, 2 obsolete files)
2.95 KB,
patch
|
Details | Diff | Splinter Review | |
840 bytes,
text/html
|
Details | |
119.92 KB,
text/plain
|
Details | |
21.41 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
I'll try using surkovs handy gCSSTextAttrsMap
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED
Comment 2•15 years ago
|
||
Hmmm... well that didn't work.
Updated•15 years ago
|
Assignee: david.bolter → nobody
Status: ASSIGNED → NEW
Comment 3•15 years ago
|
||
What's the testcase to reproduce the problem this bug is meant to describe? Aaron, could you attach a simple testcase?
Assignee | ||
Comment 4•15 years ago
|
||
(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•15 years ago
|
||
Comment on attachment 362568 [details] [diff] [review] mochitest Agreed, this should be OK to cover this case.
Assignee | ||
Comment 6•15 years ago
|
||
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•15 years ago
|
||
> 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•15 years ago
|
||
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
Comment 9•15 years ago
|
||
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.
Comment 11•15 years ago
|
||
See bug 77882?
Assignee | ||
Comment 12•15 years ago
|
||
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".
Assignee | ||
Comment 13•15 years ago
|
||
Assignee: jdaggett → surkov.alexander
Status: NEW → ASSIGNED
Attachment #362699 -
Flags: superreview?(roc)
Attachment #362699 -
Flags: review?(marco.zehe)
Assignee | ||
Updated•15 years ago
|
Attachment #362699 -
Flags: review?(david.bolter)
Assignee | ||
Comment 14•15 years ago
|
||
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 15•15 years ago
|
||
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?
Assignee | ||
Comment 16•15 years ago
|
||
(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•15 years ago
|
||
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+
Comment 18•15 years ago
|
||
Surkov, I can confirm tests pass on Mac, but unfortunately there is borkage on Linux. I attach the relevant mochitest log here.
Updated•15 years ago
|
Attachment #362699 -
Flags: review?(david.bolter)
Assignee | ||
Comment 19•15 years ago
|
||
(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+
Comment 20•15 years ago
|
||
(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.
Assignee | ||
Comment 21•15 years ago
|
||
Btw, Zoom doesn't affect on font size and font weight.
Assignee | ||
Comment 22•15 years ago
|
||
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?
I don't know.
Comment 24•15 years ago
|
||
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?
Assignee | ||
Comment 25•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 27•15 years ago
|
||
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.
Assignee | ||
Comment 28•15 years ago
|
||
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."
Assignee | ||
Comment 29•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #365162 -
Flags: review?(david.bolter)
Assignee | ||
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Comment 30•15 years ago
|
||
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 31•15 years ago
|
||
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).
Assignee | ||
Comment 33•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #365366 -
Flags: review?(mozbugz) → review+
Comment 34•15 years ago
|
||
Comment on attachment 365366 [details] [diff] [review] patch3 Looks good, thanks.
Assignee | ||
Comment 35•15 years ago
|
||
checked in http://hg.mozilla.org/mozilla-central/rev/9be036855586 thanks for reviews!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 36•15 years ago
|
||
(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.
Description
•