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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 1

[fantasai]

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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, fantasai. Okay, I keep current implementation for now.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fantasai:

When you think we should fix this bug, let me know.

Comment 4

[Masatoshi Kimura [:emk]]

Given that CSS3 Text Decoration has reached CR, I think we should just proceed instead of waiting for fantasai forever.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Agreed.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

* Add "blink" value is one of line-style value.
* Remove -moz-text-blink property.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 786756 [details] [diff] [review]
Patch

r=dbaron

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ac3d57b445

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2ac3d57b445

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

dom/imptests/editing/implementation.js is a read-only copy of the upstream test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a45f43cb373c

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for w3c repo

Aryeh, could you please land this in https://dvcs.w3.org/hg/editing (I don't have authorization).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ms2ger, in the future please talk to people first before just backing them out.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better patch for w3c repo

untested, though

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Somebody should test the above patch, which makes the standard test a little less ridiculous.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are there any mochitest failures if you just skip changing that test?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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).

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you test?  If there aren't, you should just land without that change.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, there is no problem. I relanded the patch without change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fdf867f67d6

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thank you, much appreciated.

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(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.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fdf867f67d6

Comment 29

[Jean-Yves Perrier [:teoli]]

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

Comment 30

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26/Site_Compatibility#CSS