add 'blink' to -moz-text-decoration-line and drop -moz-text-blink

RESOLVED FIXED in mozilla26

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug, {css3, dev-doc-complete, site-compat})

Trunk
mozilla26
css3, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The latest CSS3 text decoration spec defines the text-decoraton-line value as:

none | [ underline || overline || line-through || blink ]

So, if we support it, we can drop the -moz-text-blink.

But I'm not sure if this is the stable thing. And "text-decoration-line: blink" sounds the decoration lines do blink rather than the text.

fantasai: what's the state of this?
(Assignee)

Updated

5 years ago
Blocks: 104960

Comment 1

5 years ago
Previously the expansion of 'text-decoration: blink' into the longhands wasn't defined. There was a WG discussion, where we concluded to add 'blink' to text-decoration-line, since that minimizes the overhead of something that's deprecated. The status is, this hasn't had much opportunity for review. If someone has a good argument for why it should be handled differently, this could change. Probably not worth updating our implementation at this moment.

See http://lists.w3.org/Archives/Public/www-style/2012Aug/0897.html for CSSWG discussion.
(Assignee)

Comment 2

5 years ago
Thank you, fantasai. Okay, I keep current implementation for now.
Whiteboard: Wait to do this, see comment 1, but probably this should be fixed before uprefixing.
(Assignee)

Updated

4 years ago
Blocks: 825004
(Assignee)

Updated

4 years ago
Depends on: 857820
Summary: -moz-text-decoration-line should support blink value? → add 'blink' to -moz-text-decoration-line and drop -moz-text-blink
(Assignee)

Comment 3

4 years ago
fantasai:

When you think we should fix this bug, let me know.
Keywords: dev-doc-needed
Flags: needinfo?(fantasai.bugs)
Given that CSS3 Text Decoration has reached CR, I think we should just proceed instead of waiting for fantasai forever.
Agreed.
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 786756 [details] [diff] [review]
Patch

* Add "blink" value is one of line-style value.
* Remove -moz-text-blink property.
Attachment #786756 - Flags: review?(dbaron)
Comment on attachment 786756 [details] [diff] [review]
Patch

r=dbaron
Attachment #786756 - Flags: review?(dbaron) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ac3d57b445
Whiteboard: Wait to do this, see comment 1, but probably this should be fixed before uprefixing.
https://hg.mozilla.org/mozilla-central/rev/f2ac3d57b445
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
dom/imptests/editing/implementation.js is a read-only copy of the upstream test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a45f43cb373c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 787750 [details] [diff] [review]
patch for w3c repo

Aryeh, could you please land this in https://dvcs.w3.org/hg/editing (I don't have authorization).
Attachment #787750 - Flags: feedback?(ayg)
Ms2ger, in the future please talk to people first before just backing them out.
Flags: needinfo?(Ms2ger)
(In particular, backout isn't clearly the right way to make progress in this case; the test is clearly broken, and frankly hacks like that shouldn't be in W3C test suites, and if they are, we shouldn't be importing such test suites.)
Created attachment 787756 [details] [diff] [review]
better patch for w3c repo

untested, though
Attachment #787750 - Attachment is obsolete: true
Attachment #787750 - Flags: feedback?(ayg)
Attachment #787756 - Flags: feedback?(ayg)
Somebody should test the above patch, which makes the standard test a little less ridiculous.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #13)
> (In particular, backout isn't clearly the right way to make progress in this
> case; the test is clearly broken, and frankly hacks like that shouldn't be
> in W3C test suites, and if they are, we shouldn't be importing such test
> suites.)

OK, I suppose maybe backout was the right thing, but:

 * please email people when you back them out (as a general rule).  Backing people out shouldn't be bugmail-only communication.  I should have seen email about this and not just seen it when skimming bugmail)

 * if getting the test fixed in the w3c repository can't be done promptly, then we're going to modify the test.  I'm not going to let you block progress if upstream isn't responsive.

 * I apologize for not catching this during my review.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #14)
> Created attachment 787756 [details] [diff] [review]
> better patch for w3c repo
> 
> untested, though

And I did test this now; it shows the same set of failures locally with and without the patch.
(Assignee)

Comment 18

4 years ago
I feel it's odd. Are the tests of W3C created for passing on all browsers?? I don't understand why there are the code *for* Firefox. It blocks our change for improving the behavior actually.
Comment on attachment 787756 [details] [diff] [review]
better patch for w3c repo

.getAttribute() returns the unparsed string, right?  So won't this fail on, I don't know, style="tExT-dEcOrAtION\t:\n/*comment*/blink;ignored-parse-error" or whatever weird and wonderful things the CSS grammar tolerates here?  That's the whole reason I used .style to begin with.  Of course, my tests probably don't actually use such weird stuff, so if there's no other feasible way to do it I could take this patch, but I tried to write things in a way that would produce the specified result on as broad a range of input as possible.

I'm not sure this actually needs to be updated right now (see below).  If it does, I'm happy to take any patch that improves things here and doesn't break the latest publicly-available version of any browser.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18 JST) from comment #18)
> I feel it's odd. Are the tests of W3C created for passing on all browsers??
> I don't understand why there are the code *for* Firefox. It blocks our
> change for improving the behavior actually.

This part of implementation.js is not used to run the conformance tests, it's mostly just used to generate the expected output.  The tests then just populate the contenteditable region, run the commands, and check that the actual output matches expected.  If Firefox changes in a way that breaks the bulk of implementation.js, the most it could do is break regeneration of tests, which is moot because they're not being updated anyway, and if they were it would be by me and it would be my problem to fix upstream.  IIRC, test generation only works properly in Firefox and Chrome to begin with.

Given that, I don't know why it was necessary to update this at all.  I might be misremembering, but I'm pretty sure nothing in the tests we run should run this function.  Was the old implementation of isSimpleModifiableElement() actually causing test failures for us?  If so, which ones?
Attachment #787756 - Flags: feedback?(ayg)
Are there any mochitest failures if you just skip changing that test?
Flags: needinfo?(masayuki)
(In reply to :Aryeh Gregor from comment #19)
> .getAttribute() returns the unparsed string, right?  So won't this fail on,
> I don't know,
> style="tExT-dEcOrAtION\t:\n/*comment*/blink;ignored-parse-error" or whatever
> weird and wonderful things the CSS grammar tolerates here?  That's the whole
> reason I used .style to begin with.  Of course, my tests probably don't

Ah, I guess replacing getAttribute("style") with style.cssText is likely to help in that regard, then.  Though I'm not sure how the implementation of that varies across browsers.
(Assignee)

Comment 22

4 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #20)
> Are there any mochitest failures if you just skip changing that test?

I've never tested. I changed it mechanically (found with grep).
Flags: needinfo?(masayuki)
Could you test?  If there aren't, you should just land without that change.
Flags: needinfo?(masayuki)
(Assignee)

Comment 24

4 years ago
I've posted to tryserver:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8008f41bc105

I'm going go out for "Maker Party". So, I'm offline today.
Flags: needinfo?(masayuki)
(Assignee)

Comment 25

4 years ago
Okay, there is no problem. I relanded the patch without change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fdf867f67d6
Thank you, much appreciated.
Flags: needinfo?(Ms2ger)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #21)
> Ah, I guess replacing getAttribute("style") with style.cssText is likely to
> help in that regard, then.  Though I'm not sure how the implementation of
> that varies across browsers.

In my experience, these things vary pretty horribly.  I'm not sure if I ever tested .cssText specifically.
https://hg.mozilla.org/mozilla-central/rev/5fdf867f67d6
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-line
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-text-blink
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26
Keywords: dev-doc-needed → dev-doc-complete
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26/Site_Compatibility#CSS
Keywords: site-compat

Updated

3 years ago
Flags: needinfo?(fantasai.bugs)
You need to log in before you can comment on or make changes to this bug.