Closed Bug 812995 Opened 12 years ago Closed 11 years ago

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

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:
mozilla26

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: css3, dev-doc-complete, site-compat)

Attachments

(2 files, 1 obsolete file)

Patch
31.57 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
better patch for w3c repo
1.91 KB, patch
Details | Diff | Splinter Review 
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?
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.
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.
Summary: -moz-text-decoration-line should support blink value? → add 'blink' to -moz-text-decoration-line and drop -moz-text-blink
fantasai:

When you think we should fix this bug, let me know.
Flags: needinfo?(fantasai.bugs)
Given that CSS3 Text Decoration has reached CR, I think we should just proceed instead of waiting for fantasai forever.
Status: NEW → ASSIGNED
Attached patch Patch — — Splinter Review 
* Add "blink" value is one of line-style value.
* Remove -moz-text-blink property.
Attachment #786756 - Flags: review?(dbaron)
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
Closed: 11 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 → ---
Attached patch patch for w3c repo (obsolete) — — Splinter Review 
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.)
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.
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.
(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)
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)
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
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Flags: needinfo?(fantasai.bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: