Closed
Bug 1454883
Opened 6 years ago
Closed 6 years ago
Update font-stretch to css-fonts-4
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(5 files)
31.27 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
2.35 KB,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
14.11 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
1002 bytes,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
This is blocked on making font-stretch its own type for bug 1436061.
Assignee | ||
Comment 1•6 years ago
|
||
Pending review comments and fixing other misc stuff potentially, since this was mostly untested. Just don't want to lose track of it.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 8969257 [details] Bug 1454883: Update font-stretch to css-fonts-4. Wrong bug...
Attachment #8969257 -
Attachment is obsolete: true
Attachment #8969257 -
Flags: review?(karlt)
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8969257 [details] Bug 1454883: Update font-stretch to css-fonts-4. https://reviewboard.mozilla.org/r/237982/#review243986 r=me with the comments addressed. ::: servo/components/style/properties/gecko.mako.rs:2614 (Diff revision 2) > let weight: f32 = unsafe { Gecko_FontWeight_ToFloat(self.gecko.mFont.weight) }; > longhands::font_weight::computed_value::T(weight) > } > > + pub fn set_font_stretch(&mut self, v: longhands::font_stretch::computed_value::T) { > + self.gecko.mFont.stretch = (v.0).0 as i16; As far as you still need casting, this is wrong. It should be multiplied by 100 before casting to i16, otherwise it's likely breaking existing behavior. When the C++ code eventually support float stretch, the casting as well as the multiply should be removed together. ::: servo/components/style/properties/gecko.mako.rs:2622 (Diff revision 2) > + > + pub fn clone_font_stretch(&self) -> longhands::font_stretch::computed_value::T { > + use values::computed::Percentage; > + use values::generics::NonNegative; > + > + let stretch = self.gecko.mFont.stretch as f32; Likewise, this should divide 100 after converting to f32. ::: servo/components/style/values/computed/font.rs:33 (Diff revision 2) > use values::specified::font::{self as specified, MIN_FONT_WEIGHT, MAX_FONT_WEIGHT}; > use values::specified::length::{FontBaseSize, NoCalcLength}; > > pub use values::computed::Length as MozScriptMinSize; > pub use values::specified::font::{FontSynthesis, MozScriptSizeMultiplier, XLang, XTextZoom}; > +pub use values::computed::NonNegativePercentage as FontStretch; I would suggest you wrap it in a struct so that we may implement ToCss for it which restores the keywords for those specific values. ::: servo/components/style/values/computed/percentage.rs:71 (Diff revision 2) > +impl ToAnimatedValue for NonNegativePercentage { > + type AnimatedValue = Percentage; > + > + #[inline] > + fn to_animated_value(self) -> Self::AnimatedValue { > + self.0 > + } > + > + #[inline] > + fn from_animated_value(animated: Self::AnimatedValue) -> Self { > + NonNegative(animated.clamp_to_non_negative()) > + } > +} We should probably make this derivable... Maybe we can add a trait for `clamp_to_non_negative`? ::: servo/ports/geckolib/glue.rs:5442 (Diff revision 2) > + if font.font_stretch.get_system().is_some() { > + return false; > + } > + stretch.set_from(&font.font_stretch); Have a feeling that using match here like the following would make the style look more consistent ```rust match font.font_stretch.get_system() { Some(_) => return false, None => stretch.set_from(&font.font_stretch), } ``` but it doesn't matter much.
Attachment #8969257 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8969738 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → emilio
Comment 7•6 years ago
|
||
The way we ended up doing this, these patches need to land before part 3 in bug 1436048.
Blocks: 1436048
Updated•6 years ago
|
Attachment #8969738 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8970092 -
Flags: review?(hikezoe)
Assignee | ||
Comment 9•6 years ago
|
||
This was surprisingly caught by a test.
Attachment #8970096 -
Flags: review?(xidorn+moz)
Comment 10•6 years ago
|
||
Comment on attachment 8970092 [details] [diff] [review] Fix animation tests to account for font-stretch animating as a percentage. Review of attachment 8970092 [details] [diff] [review]: ----------------------------------------------------------------- This change looks reasonable to me. (though the commit message should be the same as the comment for this attachment (i.e. 'Fix animation tests to account...')). But one thing I am concerned is that we will ship this feature without the pref? If we had the pref, we have to set it in some .ini files, if not (it seems not), the animation behavior will be changed, I think we should add the pref. That's said, font-stretch is probably not so popular for animations, so it wouldn't be a big problem. Anyway, I defer to Brian's opinion.
Attachment #8970092 -
Flags: review?(hikezoe)
Attachment #8970092 -
Flags: review?(bbirtles)
Attachment #8970092 -
Flags: review+
Assignee | ||
Comment 11•6 years ago
|
||
It's not clear to me what pref are you talking about. This enables fancy variable font usage, but it's otherwise unrelated to var fonts. The idea is not having this gated by any pref.
Flags: needinfo?(hikezoe)
Comment 12•6 years ago
|
||
Comment on attachment 8970092 [details] [diff] [review] Fix animation tests to account for font-stretch animating as a percentage. Review of attachment 8970092 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/smil/test/db_smilCSSFromTo.js @@ +298,3 @@ > new AnimTestcaseFromTo("expanded", "ultra-expanded", > + { fromComp: "125%", > + midComp: "162.5%", My concern is here. The animation value at 50% position will be changed to a bigger value, 162.5%, no? Before the change (I am not sure which one is actually), the middle value of the interpolation between 'expanded' and 'ultra-expanded' is 'extra-expanded' (i.e. 150%). Am I missing something?
Comment 13•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9) > Created attachment 8970096 [details] [diff] [review] > Let inspector know that font-stretch accepts percentages. > > This was surprisingly caught by a test. Wait, what? What test caught this?
Comment 14•6 years ago
|
||
Comment on attachment 8970092 [details] [diff] [review] Fix animation tests to account for font-stretch animating as a percentage. As per a brief chat with Emilio on IRC, I think this change is fine since Chrome and Safari have already shipped this (Emilio told me that.).
Flags: needinfo?(hikezoe)
Attachment #8970092 -
Flags: review?(bbirtles)
Updated•6 years ago
|
Attachment #8970096 -
Flags: review?(xidorn+moz) → review+
Comment 15•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/87886520c599 Update font-stretch to css-fonts-4. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1091690e59 Only parse font-stretch keywords in the font shorthand. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/46996fb339f8 Fix animation tests to account for font-stretch animating as percentage. r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/cb740fda0906 Let inspector know that font-stretch supports percentages. r=xidorn
Comment 16•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ded40b73ba23 Regenerate the devtools CSS database. r=me on a CLOSED TREE
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10590 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87886520c599 https://hg.mozilla.org/mozilla-central/rev/9f1091690e59 https://hg.mozilla.org/mozilla-central/rev/46996fb339f8 https://hg.mozilla.org/mozilla-central/rev/cb740fda0906 https://hg.mozilla.org/mozilla-central/rev/ded40b73ba23
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•