Closed
Bug 1038663
Opened 9 years ago
Closed 8 years ago
Allow percentage values for 'word-spacing'
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: sebo, Assigned: n.nethercote)
References
(Blocks 2 open bugs, )
Details
(Keywords: dev-doc-complete)
Attachments
(8 files, 5 obsolete files)
7.06 KB,
patch
|
heycam
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
2.18 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.27 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.39 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
9.83 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.51 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
8.22 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
As of the current draft of the CSS Text Module Level 3 the 'word-spacing' property allows percentual values. This should also be marked as blocker for bug 913153. Sebastian
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•9 years ago
|
Blocks: css-text-3
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Comment 1•8 years ago
|
||
css-text-3 says that word-spacing always computes to an absolute length, but I don't think that makes sense for percentage values. I mailed www-style about it: https://lists.w3.org/Archives/Public/www-style/2015Oct/0196.html
![]() |
Assignee | |
Comment 2•8 years ago
|
||
This makes mWordSpacing a lot more like mLetterSpacing, while maintaining the existing "'normal' computes to 0px" behaviour.
Attachment #8677241 -
Flags: review?(cam)
Comment 3•8 years ago
|
||
Comment on attachment 8677241 [details] [diff] [review] (part 1) - Make nsStyleText::mWordSpacing an nsStyleCoord Review of attachment 8677241 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleStruct.h @@ +1686,5 @@ > uint8_t mControlCharacterVisibility; // [inherited] see nsStyleConsts.h > int32_t mTabSize; // [inherited] see nsStyleConsts.h > > + nsStyleCoord mWordSpacing; // [inherited] coord, normal > + nsStyleCoord mLetterSpacing; // [inherited] coord, normal Remove these "normal"s; we should just mention the type of values that can be stored in the nsStyleCoord.
Attachment #8677241 -
Flags: review?(cam) → review+
![]() |
Assignee | |
Comment 4•8 years ago
|
||
> > + nsStyleCoord mWordSpacing; // [inherited] coord, normal
> > + nsStyleCoord mLetterSpacing; // [inherited] coord, normal
>
> Remove these "normal"s; we should just mention the type of values that can
> be stored in the nsStyleCoord.
mLetterSpacing can still be normal, so I'll leave that one.
![]() |
Assignee | |
Comment 5•8 years ago
|
||
To implement percentage units for word-spacing, I need to be able to get the width of a space character and then multiply that width by the percentage. jtd, is there a way to get that width in either WordSpacing() (earlier) or in PropertyProvider::GetSpacingInternal() (later)? (Actually, for full CSS3 compliance we probably want to get it in GetSpacingInternal() because we need to handle unusual word-breaking chars like the Aegean Word Separator, which is not in the BMP and so I'm not sure how to detect it within IsCSSWordSpacingSpace() given that nsTExtFragment is UCS-2...)
Flags: needinfo?(jdaggett)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06cf7b88a3e5480b2d59a1330031dab67913ccbe Bug 1038663 (part 1) - Make nsStyleText::mWordSpacing an nsStyleCoord. r=heycam.
![]() |
Assignee | |
Updated•8 years ago
|
Keywords: leave-open
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8677241 -
Flags: checkin+
![]() |
Assignee | |
Comment 8•8 years ago
|
||
No response from jtd after a week. jkew, do you know the answer to the question in comment 5?
Flags: needinfo?(jfkthame)
Comment 9•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > To implement percentage units for word-spacing, I need to be able to get the > width of a space character and then multiply that width by the percentage. > jtd, is there a way to get that width in either WordSpacing() (earlier) or > in PropertyProvider::GetSpacingInternal() (later)? Within GetSpacingInternal(), it shouldn't be a problem.... IIRC, the PropertyProvider has a reference to the textrun, and from the textrun you can get a fontmetrics and thence the width of a space. Compare the ComputeTabWidthAppUnits() helper in nsTextFrame.cpp. If you need widths of other word-separating characters, you'll need to do a bit more work (space itself is easy because it's cached directly in the font metrics). Look at GetHyphenWidth(), which calls into the fontGroup to make a temporary textrun -- which handles finding the appropriate font, given that the character might not be supported by the first font in the list, so fallback may be needed -- and then measures the result; this could be generalized to take a desired character as a parameter, I guess. (If you prefer to have the width early, in WordSpacing(), I think you'd want to add the textrun as a parameter there, and then all the above applies just the same.) > (Actually, for full CSS3 compliance we probably want to get it in > GetSpacingInternal() because we need to handle unusual word-breaking chars > like the Aegean Word Separator, which is not in the BMP and so I'm not sure > how to detect it within IsCSSWordSpacingSpace() given that nsTExtFragment is > UCS-2...) (To be more accurate, it's UTF-16, not UCS-2.) To detect that, first change |ch| from a |char16_t| to |uint32_t|. Then, before the existing |switch (ch)|, you can do something like if (NS_IS_HIGH_SURROGATE(ch) && aPos + 1 < aFrag->GetLength() && NS_IS_LOW_SURROGATE(aFrag->CharAt(aPos + 1))) { ch = SURROGATE_TO_UCS4(ch, aFrag->CharAt(aPos + 1)); } so that the switch will see the full Unicode character code.
Flags: needinfo?(jfkthame)
![]() |
Assignee | |
Comment 10•8 years ago
|
||
Thank you for the detailed answer! Very helpful.
Flags: needinfo?(jdaggett)
![]() |
Assignee | |
Comment 11•8 years ago
|
||
This is necessary for a subsequent patch.
Attachment #8683411 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 12•8 years ago
|
||
This will be reused in a subsequent patch.
Attachment #8683412 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 13•8 years ago
|
||
This doesn't make much sense by itself, but one of the call sites will be changed in a subsequent patch.
Attachment #8683413 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 14•8 years ago
|
||
Again, this doesn't make sense in isolation, but it will make it easier to allow for percentage word-spacing in a subsequent patch.
Attachment #8683414 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 15•8 years ago
|
||
Attachment #8683416 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 16•8 years ago
|
||
I did my best to follow the test format rules at https://wiki.csswg.org/test/format, but I may well have gotten something wrong.
Attachment #8683417 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8683411 -
Flags: review?(cam) → review+
Updated•8 years ago
|
Attachment #8683412 -
Flags: review?(cam) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8683413 [details] [diff] [review] (part 4) - Inline StyleToCoord() Review of attachment 8683413 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.cpp @@ +1620,5 @@ > if (!aStyleText) { > aStyleText = aFrame->StyleText(); > } > + > + nsStyleCoord coord = aStyleText->mLetterSpacing; Make this a |const nsStyleCoord&| so we don't have to make a copy of it. @@ +1637,5 @@ > if (!aStyleText) { > aStyleText = aFrame->StyleText(); > } > + > + nsStyleCoord coord = aStyleText->mWordSpacing; And here.
Attachment #8683413 -
Flags: review?(cam) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8683414 [details] [diff] [review] (part 5) - Change GetSpacingFlags() Review of attachment 8683414 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these things fixed. ::: layout/generic/nsTextFrame.cpp @@ +1647,5 @@ > > +// Returns gfxTextRunFactory::TEXT_ENABLE_SPACING if non-standard > +// letter-spacing or word-spacing is present. > +static uint32_t > +GetSpacingFlags(nsIFrame* aFrame, const nsStyleText* aStyleText = nullptr) aStyleText argument is not needed. @@ +1660,5 @@ > + nsStyleCoord coord = aStyleText->mLetterSpacing; > + if (eStyleUnit_Coord == coord.GetUnit()) { > + return coord.GetCoordValue() != 0 > + ? gfxTextRunFactory::TEXT_ENABLE_SPACING > + : 0; I don't think we want to return 0 here if mLetterSpacing is 0; we need to continue to check mWordSpacing.
Attachment #8683414 -
Flags: review?(cam) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8683416 [details] [diff] [review] (part 6) - Allow percentage values for 'word-spacing' Review of attachment 8683416 [details] [diff] [review]: ----------------------------------------------------------------- I think we want to allow values like calc(50% + 2px). There are a couple of changes needed for this: ::: layout/generic/nsTextFrame.cpp @@ +1646,5 @@ > return coord.GetCoordValue(); > } > + if (eStyleUnit_Percent == coord.GetUnit()) { > + return GetSpaceWidthAppUnits(aTextRun) * coord.GetPercentValue(); > + } You will need to handle eStyleUnit_Calc values in here. You can use nsRuleNode::ComputeCoordPercentCalc to handle eStyleUnit_Percent and eStyleUnit_Calc simultaneously. (You could also use it for plain eStyleUnit_Coord values, but it might be worth skipping the GetSpaceWidthAppUnits() call for that case.) ::: layout/style/nsCSSPropList.h @@ +3602,5 @@ > WordSpacing, > CSS_PROPERTY_PARSE_VALUE | > CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE | > CSS_PROPERTY_APPLIES_TO_PLACEHOLDER | > CSS_PROPERTY_UNITLESS_LENGTH_QUIRK, Add CSS_PROPERTY_STORES_CALC here, so that animations between calc() values that have percentages in them will work. ::: layout/style/nsRuleNode.cpp @@ +4363,1 @@ > SETCOORD_CALC_LENGTH_ONLY | SETCOORD_UNSET_INHERIT, Add SETCOORD_STORE_CALC so that SetCoord stores calc values in the nsStyleCoord, rather than only accept lengths in calc() and resolve them down to an nscoord. ::: layout/style/nsStyleStruct.h @@ +1685,5 @@ > uint8_t mTextCombineUpright; // [inherited] see nsStyleConsts.h > uint8_t mControlCharacterVisibility; // [inherited] see nsStyleConsts.h > int32_t mTabSize; // [inherited] see nsStyleConsts.h > > + nsStyleCoord mWordSpacing; // [inherited] coord, percent Mention calc in the comment. ::: layout/style/test/property_database.js @@ +3722,5 @@ > type: CSS_TYPE_LONGHAND, > initial_values: [ "normal", "0", "0px", "-0em", > "calc(-0px)", "calc(0em)" > ], > + other_values: [ "1em", "2px", "-3px", "0%", "50%", Add a negative percentage value as well as some calc() values with percentages in them.
Attachment #8683416 -
Flags: review?(cam)
Comment 20•8 years ago
|
||
Comment on attachment 8683417 [details] [diff] [review] (part 7) - Add test for percentage values for 'word-spacing' Review of attachment 8683417 [details] [diff] [review]: ----------------------------------------------------------------- I think you might have to follow what's been done in the submitted/variables/ and submitted/flexbox/ tests, and that is to make a copy of Ahem.ttf, a small style sheet to reference it, stick them in a support/ subdirectory, then add a <link rel="stylesheet" href="support/ahem.css" type="text/css"> to the test file. This isn't ideal, but I don't think the test harness has the ability to ensure an Ahem font is available by default (as the CSSWG test suite assumes). ::: layout/reftests/w3c-css/submitted/text3/text-word-spacing-001.html @@ +1,3 @@ > +<!DOCTYPE html> > +<title>CSS Text Test: Word Spacing</title> > +<link rel="author" title="Nicholas Nethercote" href="nnethercote@mozilla.com"> mailto: before your email address. @@ +9,5 @@ > +div.wsn40 { word-spacing: -40%; } > +div.ws0 { word-spacing: 0%; } > +div.ws25 { word-spacing: 25%; } > +div.ws100 { word-spacing: 100%; } > +div.ws300 { word-spacing: 300%; } Please add some sub-tests for calc() values with percentages in them. @@ +19,5 @@ > + <div class="ws0" >A Bc Def Ghij</div> > + <div class="ws100" >A Bc Def Ghij</div> > + <div class="wsn40"> A Bc Def Ghij</div> > + <div class="ws300" >A Bc Def Ghij</div> > + <div class="ws25" >A Bc Def Ghij</div> These tests look great!
Attachment #8683417 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 21•8 years ago
|
||
> I think you might have to follow what's been done in the
> submitted/variables/ and submitted/flexbox/ tests, and that is to make a
> copy of Ahem.ttf, a small style sheet to reference it, stick them in a
> support/ subdirectory, then add a <link rel="stylesheet"
> href="support/ahem.css" type="text/css"> to the test file. This isn't
> ideal, but I don't think the test harness has the ability to ensure an Ahem
> font is available by default (as the CSSWG test suite assumes).
Even though I mentioned Ahem in the flags I ended up not using it, so I think I can skip all this (and update the flags).
![]() |
Assignee | |
Comment 22•8 years ago
|
||
I fixed the problems with GetSpacingFlags(). Carrying over r+.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8683414 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
Comment on attachment 8683934 [details] [diff] [review] (part 5) - Change GetSpacingFlags() >+ nsStyleCoord ls = styleText->mLetterSpacing; >+ nsStyleCoord ws = styleText->mWordSpacing; Make these |const nsStyleCoord&|.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8683934 -
Flags: review+
![]() |
Assignee | |
Comment 24•8 years ago
|
||
Attachment #8683940 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8683416 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 25•8 years ago
|
||
Attachment #8683941 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8683417 -
Attachment is obsolete: true
Comment 26•8 years ago
|
||
Comment on attachment 8683940 [details] [diff] [review] (part 6) - Allow percentage values for 'word-spacing' Review of attachment 8683940 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these things addressed. ::: layout/generic/nsTextFrame.cpp @@ +1665,5 @@ > const nsStyleText* styleText = aFrame->StyleText(); > const nsStyleCoord& ls = styleText->mLetterSpacing; > const nsStyleCoord& ws = styleText->mWordSpacing; > > bool nonStandardSpacing = Since calc() values with non-zero percentages and lengths still can end up resolving to 0, this makes our use of TEXT_ENABLE_SPACING less exact. TEXT_ENABLE_SPACING is just used as an optimisation to skip unnecessary work, so it's not incorrect to return it, but I'm not sure where the exact tradeoff here is. It's probably OK as you have it. Put a comment in here to point out that we can still return TEXT_ENABLE_SPACING even if a calc() value resolves to 0, but that precise use of TEXT_ENABLE_SPACING is only used to skip work unnecessary work in gfxTextRun, so it doesn't matter too much given that it should be an uncommon case. ::: layout/style/nsStyleCoord.h @@ +90,5 @@ > bool operator!=(const CalcValue& aOther) const { > return !(*this == aOther); > } > + > + bool IsZero() const { Consider renaming this to something that suggests it only looks at the component percentage/length values, and does not guarantee that the calc() value will resolve to 0.
Attachment #8683940 -
Flags: review?(cam) → review+
Comment 27•8 years ago
|
||
Comment on attachment 8683941 [details] [diff] [review] (part 7) - Add test for percentage values for 'word-spacing' Review of attachment 8683941 [details] [diff] [review]: ----------------------------------------------------------------- Can you just add one more test with a calc() value that has a length that isn't stored as 0px. (You could use a <span> in the reference image with a padding-left value of that length to shift the second word over.)
Attachment #8683941 -
Flags: review?(cam) → review+
![]() |
Assignee | |
Comment 28•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c0919101cd4615efbc9116f2f23bd4d43ae488 Bug 1038663 (part 2) - Move GetFirstFontMetrics() up. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/954e57438f5193d3537d82ffb21c155d0ee89b44 Bug 1038663 (part 3) - Factor out space width computation. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb77e5fad8cf0cd0d0daad678899a9087259d79 Bug 1038663 (part 4) - Inline StyleToCoord(). r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/f69af916125238619a05f7c4c294865450734c08 Bug 1038663 (part 5) - Change GetSpacingFlags(). r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/d176322f2d361d5259de7933a5172b86f2d69f48 Bug 1038663 (part 6) - Allow percentage values for 'word-spacing'. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/97e3492d6080a0a3d2eda07a4e36208b92758469 Bug 1038663 (part 7) - Add test for percentage values for 'word-spacing'. r=heycam.
![]() |
Assignee | |
Comment 29•8 years ago
|
||
Woo! heycam: thank you for all the assistance.
Comment 30•8 years ago
|
||
Comment on attachment 8683940 [details] [diff] [review] (part 6) - Allow percentage values for 'word-spacing' > const nsStyleCoord& coord = aStyleText->mWordSpacing; > if (eStyleUnit_Coord == coord.GetUnit()) { > return coord.GetCoordValue(); > } >+ if (eStyleUnit_Percent == coord.GetUnit() || >+ eStyleUnit_Calc == coord.GetUnit()) { >+ return nsRuleNode::ComputeCoordPercentCalc(coord, >+ GetSpaceWidthAppUnits(aTextRun)); >+ } > return 0; > } I'm not sure how expensive GetSpaceWidthAppUnits is, but it's possible to avoid it in slightly more cases by using nsStyleCoord::HasPercent or nsStyleCoord::ConvertsToLength, e.g., with something like: const nsStyleCoord& coord = aStyleText->mWordSpacing; if (coord.IsCoordPercentCalcUnit()) { nscoord pctBasis = coord.HasPercent() ? GetSpaceWidthAppUnits(aTextRun) : 0; return nsRuleNode::ComputeCoordPercentCalc(coord, pctBasis); } return 0;
![]() |
Assignee | |
Updated•8 years ago
|
Keywords: leave-open
Comment 31•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/20fd31f8df9a6ce32b51715a6367c81bcfc4cfd1 Backed out 6 changesets (bug 1038663) for Android opt R2 bustage
Comment 32•8 years ago
|
||
A link to the failure: http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-inbound-android-api-11/1446788323/mozilla-inbound_ubuntu64_vm_armv7_large_test-plain-reftest-2-bm68-tests1-linux64-build360.txt.gz&only_show_unexpected=1
Updated•8 years ago
|
Flags: needinfo?(n.nethercote)
Comment 33•8 years ago
|
||
Switching the test to use a monospace font might help.
![]() |
Assignee | |
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fb037c6899a58b024117475c8e420f76015986 Bug 1038663 (part 2, attempt 2) - Move GetFirstFontMetrics() up. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/5e28e1cc216cbcf31b47e46039798e2f5cc5ba31 Bug 1038663 (part 3, attempt 2) - Factor out space width computation. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/e4bf6cb201337516a1ea5b3011e7ee370d615fab Bug 1038663 (part 4, attempt 2) - Inline StyleToCoord(). r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/6858f02c757ae2c76974c45fb80e0870b85c8628 Bug 1038663 (part 5, attempt 2) - Change GetSpacingFlags(). r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc83208c0605968bbc5a451836ee2181db9771d Bug 1038663 (part 6, attempt 2) - Allow percentage values for 'word-spacing'. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/f8fa4680e7321306c8ed56c4881851fd78226bc8 Bug 1038663 (part 7, attempt 2) - Add test for percentage values for 'word-spacing'. r=heycam.
![]() |
Assignee | |
Comment 35•8 years ago
|
||
Using a monospace font fixed the Android failure. I also made the change that dbaron suggested in comment 30 before re-landing.
Comment 36•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/797d3b54d0db543988f97f42e1a819e35179d738 Backed out changeset f8fa4680e732 (bug 1038663) for OS X reftest bustage on a CLOSED TREE
![]() |
Assignee | |
Comment 37•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #35) > Using a monospace font fixed the Android failure. ... but it introduced a similar failure on OS X: http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64/1447030295/mozilla-inbound_snowleopard_test-reftest-bm107-tests1-macosx-build3482.txt.gz&only_show_unexpected=1 At this point I think I'll go back to a non-monospace font and use some fuzzy matching on Android.
Comment 38•8 years ago
|
||
Can you instead restructure the test so that it forms the same text run separation in both test and reference? (That seems likely to be the problem; there are probably sub-pixel positioning differences as a result.) Alternatively, use the Ahem font, with a font size that's a multiple of 5 pixels. I think one of those would be better than having to adjust fuzz as platforms change over time.
![]() |
Assignee | |
Comment 39•8 years ago
|
||
Attachment #8684755 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(n.nethercote)
![]() |
Assignee | |
Comment 40•8 years ago
|
||
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #38) > Can you instead restructure the test so that it forms the same text run > separation in both test and reference? Sorry, I don't understand this suggestion. > Alternatively, use the Ahem font, with a font size that's a multiple of 5 > pixels. A multiple of 5 so that -40% is a round number?
Comment 41•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #40) > (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #38) > > Can you instead restructure the test so that it forms the same text run > > separation in both test and reference? > > Sorry, I don't understand this suggestion. On second thoughts, I'm not even sure it makes sense. I could imagine extra word spaces implying a break in shaping, or not doing so. I was thinking they didn't -- but if they didn't, then the test wouldn't be failing because of that. It's probably failing because of assumptions about rounding, which means using Ahem is probably the right way to go. > > Alternatively, use the Ahem font, with a font size that's a multiple of 5 > > pixels. > > A multiple of 5 so that -40% is a round number? That's good too, but I was actually saying that because things (e.g., descent is 20%, ascent is 80%) that all are integral numbers of pixels when the size is a multiple of 5px. But yes, you probably do want your word-spacing percentages to come out reasonably round, ideally integral pixels.
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7fb037c6899 https://hg.mozilla.org/mozilla-central/rev/5e28e1cc216c https://hg.mozilla.org/mozilla-central/rev/e4bf6cb20133 https://hg.mozilla.org/mozilla-central/rev/6858f02c757a https://hg.mozilla.org/mozilla-central/rev/3bc83208c060
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7fb037c6899 https://hg.mozilla.org/mozilla-central/rev/5e28e1cc216c https://hg.mozilla.org/mozilla-central/rev/e4bf6cb20133 https://hg.mozilla.org/mozilla-central/rev/6858f02c757a https://hg.mozilla.org/mozilla-central/rev/3bc83208c060
![]() |
Assignee | |
Comment 44•8 years ago
|
||
Now using Ahem. Works locally, wish me luck for the try run I'm about to do...
Attachment #8685091 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8684755 -
Attachment is obsolete: true
Attachment #8684755 -
Flags: review?(cam)
![]() |
Assignee | |
Comment 45•8 years ago
|
||
Oh, I forgot to make the font size a multiple of 5 pixels.
Attachment #8685105 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8685091 -
Attachment is obsolete: true
Attachment #8685091 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8685105 -
Flags: review?(cam) → review+
![]() |
Assignee | |
Comment 46•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b84d7638278ac102cb20514fe926a3a66a6c6f7 Bug 1038663 (part 7, attempt 3) - Add test for percentage values for 'word-spacing'. r=heycam.
Reporter | ||
Comment 47•8 years ago
|
||
Works fine for me. Thank you guys! Documented the changes here: https://developer.mozilla.org/en-US/docs/Web/CSS/word-spacing https://developer.mozilla.org/en-US/Firefox/Releases/45 Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 48•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b84d7638278
![]() |
Assignee | |
Comment 49•8 years ago
|
||
Thank you for the documentation, Sebastian.
Comment 50•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63a0aefa179381c388fe0ad42c615e5855a87d0 Bug 1038663 followup - Add the link rel=match that makes the CSS WG harness believe this test a reftest. No review. https://hg.mozilla.org/integration/mozilla-inbound/rev/18fb0664e59187d2a6b22384d484c8c54655262f Bug 1038663 followup - Remove specification link from reftest reference, which the CSS WG harness considers an error. No review.
Comment 51•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/26bf115402e27a4f24de8a040f8b36ac8e7d8ce4 Bug 1038663 followup - Remove assertion from reftest reference, which the CSS WG harness considers an error. No review.
![]() |
Assignee | |
Comment 52•8 years ago
|
||
Thank you for the test fixes, dbaron. I did my best to follow the format but I'm unfamiliar with it and so I'm not surprised there were minor problems.
Comment 53•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a63a0aefa179 https://hg.mozilla.org/mozilla-central/rev/18fb0664e591 https://hg.mozilla.org/mozilla-central/rev/26bf115402e2
You need to log in
before you can comment on or make changes to this bug.
Description
•