Closed Bug 1250342 Opened 8 years ago Closed 8 years ago

rename text-align(/-last): true to unsafe

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files)

MozReview Request: Bug 1250342 patch 1: Rename exposed keyword for text-align: true to unsafe. r?mats
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
MozReview Request: Bug 1250342 patch 2: Rename NS_STYLE_TEXT_ALIGN_TRUE to NS_STYLE_TEXT_ALIGN_UNSAFE. r?mats
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
MozReview Request: Bug 1250342 patch 3: Rename preference layout.css.text-align-true-value.enabled to layout.css.text-align-unsafe-value.enabled . r?mats
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
patch 4: update "text-align-true" reftest
3.56 KB, patch
Details | Diff | Splinter Review
reftest output showing the fuzzy mismatch on the serif of the "u" in "unsafe"
71.31 KB, text/plain
Details 
As discussed in bug 1237059 comment 14 and bug 1237059 comment 15, we should rename text-align: true and text-align-last: true to unsafe to match the current alignment spec at https://drafts.csswg.org/css-align/#overflow-values .
This would be a waste of time if the CSSWG decides that it doesn't want to introduce this
feature on the 'text-align' property.  I think we should wait until they respond to:
https://lists.w3.org/Archives/Public/www-style/2016Jan/0278.html
BTW, if you happen to attend a meeting where this is discussed, please bring up serialization.
It might be confusing for authors that the css-align properties suppress 'safe' b/c it's
the default, whereas 'text-align' suppress 'unsafe' b/c it's the default there.
I think it's confusing to have the old name in the tree, and trivial to fix (I wrote the patches within a few minutes after filing this bug).
Comment on attachment 8722307 [details]
MozReview Request: Bug 1250342 patch 1: Rename exposed keyword for text-align: true to unsafe.  r?mats

https://reviewboard.mozilla.org/r/35979/#review32599

LGTM, r=mats
Attachment #8722307 - Flags: review?(mats) → review+
Comment on attachment 8722308 [details]
MozReview Request: Bug 1250342 patch 2:  Rename NS_STYLE_TEXT_ALIGN_TRUE to NS_STYLE_TEXT_ALIGN_UNSAFE.  r?mats

https://reviewboard.mozilla.org/r/35981/#review32601
Attachment #8722308 - Flags: review?(mats) → review+
Comment on attachment 8722309 [details]
MozReview Request: Bug 1250342 patch 3:  Rename preference layout.css.text-align-true-value.enabled to layout.css.text-align-unsafe-value.enabled .  r?mats

https://reviewboard.mozilla.org/r/35983/#review32603
Attachment #8722309 - Flags: review?(mats) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/866f8a7337dfb9df08f8e85cb609f34a218c44d0
Bug 1250342 patch 1: Rename exposed keyword for text-align: true to unsafe.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82e430d0edad33d64dcf344696182d7333f93eb
Bug 1250342 patch 2:  Rename NS_STYLE_TEXT_ALIGN_TRUE to NS_STYLE_TEXT_ALIGN_UNSAFE.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eae0890ee1166bf9fd3454789a9f5d156f07c36
Bug 1250342 patch 3:  Rename preference layout.css.text-align-true-value.enabled to layout.css.text-align-unsafe-value.enabled .  r=mats
This landing broke the reftest for this "text-align:true" feature.

I prepared this patch locally as a quick-and-easy fix (updating the test to use the keyword), but I got a local reftest mismatch on the serif of the "u" on the word "unsafe" that is now present in the test.  Probably just needs fuzz to correct for antialiasing, but needs some treeherder diagnosis to see how much fuzz & if that's platform-specific.

Recommended that kwierso back out, in the meantime.
Attachment #8722642 - Attachment description: patch needed to update "text-align-true" reftest → patch 4: update "text-align-true" reftest
Attachment #8722642 - Attachment is patch: true
(So, before this re-lands, it probably needs a Try run with patch 4, to figure out whether/where we need fuzz to correct for that "u" serif fuzziness. Or we could just force the test to use a sans-serif font. Or maybe the fuzziness is a real bug & needs deeper investigation.)

Also, before this re-lands -- it'd probably be good to fix patch 1 to correct each now-stale mentions of "true" in comments alongside the code-changes, e.g. here:
>   if (!sIsInitialized) {
>     // First run: find the position of "true" in kTextAlignKTable.
>     sIndexOfTrueInTextAlignTable =
>-      nsCSSProps::FindIndexOfKeyword(eCSSKeyword_true,
>+      nsCSSProps::FindIndexOfKeyword(eCSSKeyword_unsafe,
>                                      nsCSSProps::kTextAlignKTable);
https://hg.mozilla.org/integration/mozilla-inbound/rev/866f8a7337dfb9df08f8e85cb609f34a218c44d0#l1.10
The reftest fuzziness after applying 'patch 4' (shown in my attached reftest log) is from a single pixel, between the "|" and the u's upper-left serif -- it's rgb(255,255,255) in the testcase vs rgb(255,255,246) in the reference.

Changing to a "sans-serif" font doesn't help -- it introduces a more severe version of the same problem - it gives me a full column of subpixel-antialiasing fuzziness to the right of the "|" character.

So we might just want to take patch 1 with a "fuzzy-if([CONDITION],9,1) annotation. Anyway, I'll stop poking around here & let dbaron address this as he sees fit.
(In reply to Daniel Holbert [:dholbert] from comment #15)
> So we might just want to take patch 1 with a "fuzzy-if([CONDITION],9,1)
> annotation.

(sorry, meant "patch 4", not "patch 1")
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48cbd30153d&selectedJob=17135604 shows that I can just reland with patch 4 (the test failures are from the other bug in that try run)
OK, my subpixel antialiasing issues must be a local quirk then.

(Still probably worth fixing the now-wrong code-comments mentioned in comment 14 before re-landing, though.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/45470ff0ca1a8394a2d0543f9403eee58d4a68c3
Bug 1250342 patch 1: Rename exposed keyword for text-align: true to unsafe.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef35da48cfd4e80a506a8fb1c8d432323eb2b9f
Bug 1250342 patch 2:  Rename NS_STYLE_TEXT_ALIGN_TRUE to NS_STYLE_TEXT_ALIGN_UNSAFE.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3a5d0dfc82dd0ca8b3b86fe8c7408b1c381d35
Bug 1250342 patch 3:  Rename preference layout.css.text-align-true-value.enabled to layout.css.text-align-unsafe-value.enabled .  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5e5a12ce6376ab68e1da6fbcef1bc690b6ff76
Bug 1250342 patch 4: Update & rename reftest 'text-align-true.html' to use 'unsafe' instead of 'true'.  r=dbaron
Oh, I forgot to do comment 14; I'll do that in a followup.
https://hg.mozilla.org/integration/mozilla-inbound/rev/87064fa7de535b7ed053933365a1d774ecbeef1a
Bug 1250342 patch 5 - Additional comment and variable name changes that should have been in patches 1 or 3.
Flags: needinfo?(dbaron)
Hmm, it just occurred to me that we don't support explicitly specifying 'safe'.
We probably should for consistency with <overflow-position> in css-align.
(In reply to Mats Palmgren (:mats) from comment #22)
> Hmm, it just occurred to me that we don't support explicitly specifying
> 'safe'.
> We probably should for consistency with <overflow-position> in css-align.

I think we may as well wait for the spec for that.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: