Last Comment Bug 812995 - add 'blink' to -moz-text-decoration-line and drop -moz-text-blink
: add 'blink' to -moz-text-decoration-line and drop -moz-text-blink
Status: RESOLVED FIXED
: css3, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla26
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
http://dev.w3.org/csswg/css-text-deco...
Depends on: 857820
Blocks: css-text-3 59109 825004
  Show dependency treegraph
 
Reported: 2012-11-19 01:02 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2014-04-24 12:48 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (31.57 KB, patch)
2013-08-06 23:12 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
dbaron: review+
Details | Diff | Review
patch for w3c repo (2.08 KB, patch)
2013-08-08 13:26 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
better patch for w3c repo (1.91 KB, patch)
2013-08-08 13:47 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-11-19 01:02:21 PST
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 2012-11-19 16:37:56 PST
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] (Mozilla Japan) 2012-11-19 19:43:43 PST
Thank you, fantasai. Okay, I keep current implementation for now.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 23:11:00 PDT
fantasai:

When you think we should fix this bug, let me know.
Comment 4 Masatoshi Kimura [:emk] 2013-08-03 00:42:51 PDT
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-7 (review requests must explain patch) 2013-08-03 13:36:19 PDT
Agreed.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-08-06 23:12:27 PDT
Created attachment 786756 [details] [diff] [review]
Patch

* Add "blink" value is one of line-style value.
* Remove -moz-text-blink property.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-07 15:50:04 PDT
Comment on attachment 786756 [details] [diff] [review]
Patch

r=dbaron
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-08-07 19:19:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ac3d57b445
Comment 9 Carsten Book [:Tomcat] 2013-08-08 04:17:12 PDT
https://hg.mozilla.org/mozilla-central/rev/f2ac3d57b445
Comment 10 :Ms2ger 2013-08-08 10:42:39 PDT
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-7 (review requests must explain patch) 2013-08-08 13:26:14 PDT
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).
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-08 13:33:49 PDT
Ms2ger, in the future please talk to people first before just backing them out.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-08 13:35:43 PDT
(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-7 (review requests must explain patch) 2013-08-08 13:47:45 PDT
Created attachment 787756 [details] [diff] [review]
better patch for w3c repo

untested, though
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-08 13:48:48 PDT
Somebody should test the above patch, which makes the standard test a little less ridiculous.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-08 14:01:20 PDT
(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-7 (review requests must explain patch) 2013-08-08 14:03:59 PDT
(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] (Mozilla Japan) 2013-08-08 17:19:23 PDT
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 (away until August 15) 2013-08-09 04:35:18 PDT
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-7 (review requests must explain patch) 2013-08-09 15:09:57 PDT
Are there any mochitest failures if you just skip changing that test?
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-09 15:12:25 PDT
(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] (Mozilla Japan) 2013-08-09 16:43:01 PDT
(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-7 (review requests must explain patch) 2013-08-09 16:53:22 PDT
Could you test?  If there aren't, you should just land without that change.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-08-09 16:59:14 PDT
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] (Mozilla Japan) 2013-08-10 16:24:34 PDT
Okay, there is no problem. I relanded the patch without change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fdf867f67d6
Comment 26 :Ms2ger 2013-08-11 02:38:47 PDT
Thank you, much appreciated.
Comment 27 :Aryeh Gregor (away until August 15) 2013-08-11 04:41:43 PDT
(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] 2013-08-12 03:52:20 PDT
https://hg.mozilla.org/mozilla-central/rev/5fdf867f67d6

Note You need to log in before you can comment on or make changes to this bug.