Add support for specifying 'font-stretch' in the CSS 'font' shorthand

RESOLVED FIXED in Firefox 43

Status

()

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: syoichi, Assigned: jfkthame)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [parity-blink], URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
CSS 2.1: [ [ <'font-style'> || <'font-variant'> || <'font-weight'> ]? <'font-size'> [ / <'line-height'> ]? <'font-family'> ] | caption | icon | menu | message-box | small-caption | status-bar
http://www.w3.org/TR/CSS21/fonts.html#font-shorthand

CSS 3: [ [ <‘font-style’> || <font-variant-css21> || <‘font-weight’> || <‘font-stretch’ ]? <‘font-size’> [ / <‘line-height’> ]? <‘font-family’> ] | caption | icon | menu | message-box | small-caption | status-bar
http://dev.w3.org/csswg/css-fonts-3/#font-prop

Now Gecko lacks "<font-weight>" support.
But, it seems that Blink supports new syntax parsing(I don't know about rendering).

If this feature is implemented, this MDN page should be updated.
https://developer.mozilla.org/en-US/docs/Web/CSS/font
(Reporter)

Updated

5 years ago
Blocks: css-fonts-3
Depends on: font-stretch
Whiteboard: [parity-blink]
(Reporter)

Updated

5 years ago
Keywords: testcase
(In reply to Syoichi Tsuyuhara from comment #0)
> Now Gecko lacks "<font-weight>" support.

Do you mean <font-stretch>?
(Reporter)

Comment 2

5 years ago
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #1)
> (In reply to Syoichi Tsuyuhara from comment #0)
> > Now Gecko lacks "<font-weight>" support.
> 
> Do you mean <font-stretch>?

Ah, yes. It's correct. I'm sorry.
(Reporter)

Comment 3

5 years ago
Comment on attachment 8477815 [details]
Test Case for CSS Fonts Module Level 3's font shorthand property syntax parsing(about <font-stretch>)

Fix Test Case's Description
Attachment #8477815 - Attachment description: Test Case for CSS Fonts Module Level 3's font shorthand property syntax parsing(about <font-weight>) → Test Case for CSS Fonts Module Level 3's font shorthand property syntax parsing(about <font-stretch>)
Summary: Add support for font shorthand property's new syntax → Add support for specifying 'font-stretch' in the CSS 'font' shorthand
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

4 years ago
Here's a patch to add a font-stretch keyword to the shorthand in property_database. I've checked that this causes several mochitests to fail, as expected (so it will need to be folded together with the actual code patch for landing).
Attachment #8656004 - Flags: review?(jdaggett)
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
And this should make the font shorthand accept the stretch keywords. If unit tests are happy with this, I'll also add a reftest to confirm that it has the expected effect on rendering.
Attachment #8656005 - Flags: review?(jdaggett)
(Assignee)

Comment 7

4 years ago
Oops - fixed copy/paste error in the patch.
Attachment #8656009 - Flags: review?(jdaggett)
(Assignee)

Updated

4 years ago
Attachment #8656005 - Attachment is obsolete: true
Attachment #8656005 - Flags: review?(jdaggett)

Updated

4 years ago
Attachment #8656004 - Flags: review?(jdaggett) → review+
Comment on attachment 8656009 [details] [diff] [review]
pt 2 - Add support for font-stretch values in the font shorthand

r+ with minor cleanup below

> +  // Get optional font-style, font-variant, font-weight, font-stretch
> +  // (in any order)
> +  static const nsCSSProperty fontIDs[] = {
> +    eCSSProperty_font_style,
> +    eCSSProperty_font_variant_caps,
> +    eCSSProperty_font_weight,
> +    eCSSProperty_font_stretch
> +  };

> @@ -12249,16 +12251,20 @@ CSSParserImpl::ParseFont()
>    if ((found & 4) == 0) {
>      // Provide default font-weight
>      values[2].SetIntValue(NS_FONT_WEIGHT_NORMAL, eCSSUnit_Enumerated);
>    }
> +  if ((found & 8) == 0) {
> +    // Provide default font-stretch
> +    values[3].SetIntValue(NS_FONT_STRETCH_NORMAL, eCSSUnit_Enumerated);
> +  }
 
I think it would be a good idea to tidy this code up a bit by using explicit constants (e.g. STYLE_INDEX, WEIGHT_INDEX) and a comment to indicate that the indices are dependent upon the ordering of fontIDs[].
Attachment #8656009 - Flags: review?(jdaggett) → review+

Updated

4 years ago
Attachment #8656044 - Flags: review?(jdaggett) → review+
Comment on attachment 8656045 [details] [diff] [review]
pt 3 - Reftest for use of font-stretch values in the font shorthand

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

r+ with the fix below

::: layout/reftests/font-matching/reftest.list
@@ +71,5 @@
>  HTTP(..) == stretchmapping-137.html stretchmapping-137-ref.html
>  
>  # test for font-stretch using @font-face
>  skip-if(B2G||Mulet) skip-if(Android&&AndroidVersion>15) HTTP(..) == font-stretch-1.html font-stretch-1-ref.html # bugs 773482, 927602 # Initial mulet triage: parity with B2G/B2G Desktop
> +skip-if(B2G||Mulet) skip-if(Android&&AndroidVersion>15) HTTP(..) == font-shorthand-stretch-1.html font-stretch-1-ref.html

Copy the bug numbers from the line above
Attachment #8656045 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f54b7bf6b3dbb8fb5fabe7821b21b7ca27fd23
Bug 1057680 - Add support for font-stretch values in the font shorthand. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/8408b26adf818039e669e807f20d6c2e5d7c054a
Bug 1057680 - Reftest for use of font-stretch values in the font shorthand. r=jdaggett
https://hg.mozilla.org/mozilla-central/rev/a0f54b7bf6b3
https://hg.mozilla.org/mozilla-central/rev/8408b26adf81
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
For testing: the testcase should return TRUE on all lines.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.