Closed Bug 1144885 Opened 9 years ago Closed 9 years ago

font-size-adjust:0 interpreted as font-size-adjust:none

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- verified

People

(Reporter: zwol, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: dev-doc-complete, testcase)

Attachments

(3 files, 2 obsolete files)

Treat font-size-adjust: none as separate from font-size-adjust: 0
15.83 KB, patch
jtd
: review+
Details | Diff | Splinter Review
additional changes
4.93 KB, patch
jtd
: review+
Details | Diff | Splinter Review
Add reftests for font-size:0 and font-size-adjust:0
7.38 KB, patch
jtd
: review+
Details | Diff | Splinter Review 
nsFont::sizeAdjust (https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsFont.h#93) is documented to treat 0.0 as "no adjustment", and consistent with that, 0.0 is what nsRuleNode::SetFont pokes into that field when the specified value of font-size-adjust is any of 'initial', 'none', and '0', but per https://lists.w3.org/Archives/Public/www-style/2015Mar/0282.html '0' should instead have the same effect as 'font-size: 0' (i.e. text is drawn zero pixels tall).

Regrettably I do not have a test case worked up.
font-size-adjust: <number> rejects negative numbers, so a straightforward fix would be to make the 'none' value be -1.0 instead of 0.0.
> Regrettably I do not have a test case worked up.

http://www.gtalbot.org/BrowserBugsSection/CSS3Fonts/font-size-adjust-005.xht


Other edge (negative, percent, 'auto') cases in

http://www.gtalbot.org/BrowserBugsSection/CSS3Fonts/

along with reference files

Gérard
Keywords: testcase
Version: unspecified → Trunk
Assignee: nobody → dbaron
QA Contact: dbaron
(In reply to David Baron [:dbaron] (UTC-7) from comment #3)
> Uncompiled, untested, and possibly-incomplete patch at
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> b8e7d864bd88/font-size-adjust-zero

At a skim, the various places in gfx where you changed 'sizeAdjust != 0' to 'sizeAdjust > 0' probably need to be 'sizeAdjust >= 0' for font-size-adjust:0 to have the intended effect, don't they?  Unless rendering fonts zero pixels tall is already weeded out before we get there.
(In reply to Zack Weinberg (:zwol) from comment #4)
> At a skim, the various places in gfx where you changed 'sizeAdjust != 0' to
> 'sizeAdjust > 0' probably need to be 'sizeAdjust >= 0' for
> font-size-adjust:0 to have the intended effect, don't they?  Unless
> rendering fonts zero pixels tall is already weeded out before we get there.

Those were places that already test size > 0, so the 0 pixels stuff should have been weeded out prior to that, but if it's not, we want to avoid entering that code with a 0 size.
Without the patch, the addition to property_database.js leads to
failures related to the "0.0" and "0" values in
layout/style/test/test_value_computation.html .
Attachment #8580233 - Flags: review?(jdaggett)
(I tried to push this to try, but try was closed, so I didn't.)
The failures there were due to a change I missed in StyleAnimationValue.cpp; I'll upload a new patch.
Without the patch, the addition to property_database.js leads to
failures related to the "0.0" and "0" values in
layout/style/test/test_value_computation.html .
Attachment #8580386 - Flags: review?(jdaggett)
Attachment #8580233 - Attachment is obsolete: true
Attachment #8580233 - Flags: review?(jdaggett)
Comment on attachment 8580386 [details] [diff] [review]
Treat font-size-adjust: none as separate from font-size-adjust: 0

Er, and there's something wrong on Win7 as well.
Attachment #8580386 - Flags: review?(jdaggett)
Without the patch, the addition to property_database.js leads to
failures related to the "0.0" and "0" values in
layout/style/test/test_value_computation.html .
Attachment #8581069 - Flags: review?(jdaggett)
Comment on attachment 8581069 [details] [diff] [review]
Treat font-size-adjust: none as separate from font-size-adjust: 0

Review of attachment 8581069 [details] [diff] [review]:
-----------------------------------------------------------------

r+ looks good

Do we need an extra reftest for the font-size-adjust: 0 case?
Attachment #8581069 - Flags: review?(jdaggett) → review+
I've written some reftests for both font-size:0 and font-size-adjust:0 (in w3c-css/submitted/fonts3/), and they look good on try so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa5e8dd1358f
modulo the Android orange from the other bug in that push, fixed in a later try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03ecf24c644
Gérard Talbot also wrote some tests (see link in comment 2), might be good to make sure we aren't pushing duplicate tests to the W3C.
The reftests fail on Win7 and Win8.  I'm a little surprised, but it seems like font-size-adjust:0 somehow influences line-height calculation on those platforms (when I don't think it should per spec... although I think it would be reasonable for it to do so).  Probably via GetNormalLineHeight().

I might not have time to work on that bit anytime soon...
Actually, I think I was looking at a bug for everything other than DWrite, and I need to adjust mboth code and tests.  But testing that theory...
I'll merge this in to the other patch once reviewed.

I believe the change to gfxFont::SanitizeMetrics was needed to make a
behavior difference between platforms go away; without that, some
metrics would be zero only on DWrite and not on other platforms.

Most of the other changes (except to gfxFT2FontBase) make the adjusted
size 0 more consistently when font-size-adjust is 0; with the prior
patch this was only fully true for gfxDWriteFonts.  I didn't observe any
problems related to this, but I think this probably is needed for
consistency, to keep mAdjustedSize sensible.
Attachment #8582882 - Flags: review?(jdaggett)
Attachment #8582882 - Flags: review?(jdaggett) → review+
Attachment #8582883 - Flags: review?(jdaggett) → review+
https://hg.mozilla.org/mozilla-central/rev/da024a455adc
https://hg.mozilla.org/mozilla-central/rev/1aa682e800b1
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

I just found a website [1] where "font-size-adjust: 0" was (incorrectly) used and as a result no text was showing.

I wonder why we fixed this bug; it looks like that "font-size-adjust: 0" would be only incorrectly used by an author. Do you think we could at least output a warning when this happens?


[1] http://blog.clever-age.com/fr/
(note that it will likely be fixed soon)



Release Note Request (optional, but appreciated)
[Why is this notable]: This can bring issues in existing websites.
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
David, Julien: Could you please provide a suggested wording for this bug?
Flags: needinfo?(felash)
Flags: needinfo?(dbaron)
I think it belongs in "Firefox 40 for developers", but I don't think it belongs in the release notes.
Flags: needinfo?(dbaron)
Keywords: dev-doc-needed
How about:

Since Firefox 40, `font-size-adjust: 0` is now correctly implemented and as a result websites which use this value behave as if `font-size: 0px` is used. We've found some websites incorrectly use this value and consequently all their text will be invisible. An obvious fix is to remove the value altogether.
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #29)
> How about:
> 
> Since Firefox 40, `font-size-adjust: 0` is now correctly implemented and as
> a result websites which use this value behave as if `font-size: 0px` is
> used. We've found some websites incorrectly use this value and consequently
> all their text will be invisible. An obvious fix is to remove the value
> altogether.

I tend to agree with David on this one. Perhaps this will be better captured in MDN docs or a blog post instead of release note. Release notes typically tend to be short one-liners like "HTML source can be viewed in a tab" or "Improved Performance tools in Dev edition". I'd like to remove the relnote flag from this one. Please let me know if you have any concerns.
relnote-firefox: ? → ---
I agree this should better be in "Firefox 40 for developers". Not sure what the process should be to put something there though.
QA Whiteboard: [good first verify]
(In reply to Julien Wajsberg [:julienw] from comment #31)
> I agree this should better be in "Firefox 40 for developers". Not sure what
> the process should be to put something there though.

The dev-doc-needed keyword should be sufficient.
I followed the link provided on comment 2 and reproduced this bug on Firefox nightly windows 8.1(32bit)

BUILD ID   20130318030947
User Agent  Mozilla/5.0 (Windows NT 6.2; rv:22.0) Gecko/20130318 Firefox/22.0

That link proves the fix of this bug on Latest Firefox beta

Build ID 	20150723165742
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:40.0) Gecko/20100101 Firefox/40.0

[bugday-20150724]
I followed the link provided on comment 2 and reproduced this bug on Firefox nightly windows 8.1(32bit)

BUILD ID   20130318030947
User Agent  Mozilla/5.0 (Windows NT 6.2; rv:22.0) Gecko/20130318 Firefox/22.0

That link proves the fix of this bug on Latest Firefox beta

Build ID 	20150723165742
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:40.0) Gecko/20100101 Firefox/40.0

[bugday-20150724]
> I followed the link provided on comment 2 (...)

I am setting the URL of this bug report to

http://test.csswg.org/suites/css-fonts-3_dev/nightly-unstable/html/chapter-3.htm#s3.6

so that anyone can try any or all the (now total of 7) 'font-size-adjust' tests submitted to the CSS3 Fonts Test Suite.

Marking as VERIFIED FIXED
URL: http://test.csswg.org/suites/css-fon...
Status: RESOLVED → VERIFIED
Setting the status flag as well... thank you all for verifying!
status-firefox40: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: