89065 - text-decoration:blink not working

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Derek Wueppelmann]

When using the Blink Tag or a CSS style which incorportes the Blink attribute it
is not displayed by the browser as blinking, although the text is shown.

Comment 1

[Derek Wueppelmann]

Worked fine in 0.9.1 but no longer in 0.9.2

Comment 2

[Henri Sivonen (:hsivonen)]

This is not necessarily a bad thing or a bug.

<blink> in not a real HTML 4 tag. CSS user agents aren't required to honor
text-decoration: blink. (Opera and Mac IE don't support blinking text, either.)

Comment 3

[Christopher Hoess (gone)]

Repeat after me: this is a feature, not a bug. :)

Seriously, while we shouldn't support <blink> (NS wants to support it as a 
matter of tradition, but that's just a matter of adding a line in html.css), 
text-decoration: blink did and should work; sounds like a regression in the 
Style System.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Over to the style system.  <blink> is implemented using text-decoration:blink,
so if text-decoration does not work neither will the tag.  This was working in a
May 15 build.  It's broken in a June 6 build.  Chances are, this is hyatt's
rewrite of the style system.

Comment 5

[Mike Young]

Yes, I reecho the chorus of web developers from NS2.0 "That's not a bug, its a
feature!"  (I even remember some Moz developers talking about turning it off.)

But the fact is that it *wasn't* turned off, so there is a bug here with serious
underlying ramifications.

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: confirming test case

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

**Please**, this is not the time nor the place to re-start a n-th occurence of
a religious never-ending discussion ! CSS 2 spec does not require from conforming
user agents to support 'blink' but it used to be implemented and working in Moz.
Then it is clearly a regression, period.

Comment 8

[Hixie (not reading bugmail)]

This is almost certainly hyatt's.

hyatt: you fudged about with text-decoration, and I'm guessing it broke blink.
Care to take a look? Cheers!

Comment 9

[James Barwick]

blinks in my html.css...still don't work.  I wan it! (just me whining)  Ignore me.

Comment 10

[Chase Tingley]

*** Bug 96236 has been marked as a duplicate of this bug. ***

Comment 11

[Skewer]

OS-All, this behavior is "fixed" in Windows also.

This is *not* required for CSS1, and most browsers take advantage of that
exception for an obvious reason - it's annoying. If blinking text ever works
again, we'll have to reopen bug 19258 as a regression. I strongly suggest
resolving this as WONTFIX, and since this is not required for CSS1 this is
nothing more than an "enhancement."

Comment 12

[Skewer]

Also, note that I do think any style-rendering bugs that cause text-decoration:
blink not to work should be fixed just like any other bug. But any patch that
does this should include a workaround to stop text-decoration: blink from working.

Comment 13

[Chase Tingley]

Bear with me, my knowledge of the style system is limited.  I'm pretty sure I
know what's going wrong, though.

AddBlinkFrame is called for a given text frame during in
|nsTextFrame::Reflow()| based on font data -- checking for a bit in
|ts.mFont->mFont.decorations|, where (the first) mFont is obtained from
|GetStyleData()|.

see:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#4939

However, since the rule tree landing, the appropriate bit is never being
set.  The text-decoration info is handled in
|nsRuleNode::ComputeTextResetData()|, and the bit is set in
|nsStyleTestReset::mTextDecoration| instead of in font info.  For non-blink
text decorations, |nsTextFrame::PaintTextDecorations()| walks up through
the contexts and looks at TextReset data to figure out what's going on.  The
code to handle blink is suffering from bit-rot.

I can take a stab at a patch, though perhaps not tonight.  If I do unleash this
horror upon the world once again, I'll look into the pref thing as penance --
though in a separate patch.

Comment 14

[Chase Tingley]

Attachment: patch

Comment 15

[Chase Tingley]

I used the loop in |PaintTextDecorations()| as a template.  Since |ts| is no
longer needed to look for blink, I moved several lines until after the loop in
order to keep the code coherent.

Someone please tell me if this code is bogus.  Hixie?  Hyatt?  

Comment 16

[Hixie (not reading bugmail)]

Looks reasonable -- I'm not sure what |hasDecorations| is, though, or why you need 
to stop altogether when it is false --  could you enlighten me? Thanks!

Comment 17

[Chase Tingley]

My understanding was the |context->HasTextDecorations()| (which checks for the
|NS_STYLE_HAS_TEXT_DECORATIONS| bit) could be used to avoid needlessly walking
all the way up to the top context -- the bit should be propagated to children if
there's any decoration (so if there's no bit, no chance of blink).

If this is a correct assumption, |hasDecorations| itself is extraneous -- the
check could be folded into the |while| condition.

Comment 18

[Chase Tingley]

Attachment: 2nd try - eliminate the unnecessary variable

Comment 19

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

If there's already code in the reflow state to handle blink, can't we store it
there rather than walking up the style context tree every time?

(The reflow state is a stack based object that's basically has a whole bunch of
variables that a lot of the reflow functions need (much easier to deal with, and
faster, than passing thirty parameters from function to function).  Since reflow
is recursive, it can pass state set by a parent to all of that parent's children.)

Comment 20

[Chase Tingley]

Attachment: patch - keep track of blinking in nsHTMLReflowState

Comment 21

[Chase Tingley]

This version stashes extra state in nsHTMLReflowState and inherits from the
parent.  I'm not sure how good this patch is, but at least it doesn't do a lot
of needless context tree walking.

Comment 22

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

Comment on attachment 49461 [details] [diff] [review]
patch - keep track of blinking in nsHTMLReflowState

>Index: base/public/nsHTMLReflowState.h
>+  // Keep track of text-decoration: blink
>+  PRBool              mBlinks;
>+

Could you make this a PRPackedBool and stick it after mUseAlignCharOffset?
(this will keep the size of nsHTMLReflowState 4 bytes smaller, thanks to
alignment).

>+    frame->GetStyleData(eStyleStruct_TextReset,
>+                      (const nsStyleStruct*&)st);

The indentation of the paren is off by two, and also
NS_STATIC_CAST(const nsStyleStruct*&, st) is preferable
to the old-style cast.

>+    mBlinks = (st && (st->mTextDecoration & NS_STYLE_TEXT_DECORATION_BLINK));

There's no need to null-check st.

Other than that, it looks fine.

Comment 23

[Chase Tingley]

Attachment: patch - updated as suggested by dbaron

Comment 24

[Chase Tingley]

I kept the old-style cast after a brief discussion with dbaron.  (All the cool
kids were doing it.)

Comment 25

[David Hyatt]

sr=hyatt

Comment 26

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

Comment on attachment 50041 [details] [diff] [review]
patch - updated as suggested by dbaron

r=dbaron

Comment 27

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

Fix checked in 2001-10-10 17:15 PDT.  Thanks for the patch.

Comment 28

[Skewer]

Text is blinking again in 2001101103 W98. *groan*

Comment 29

[Jacek Piskozub]

WFM, 2001101103 on WindowsME.

"Their tags shall blink until the end of days." 
                from The Book of Mozilla, 12:10 

The problem is I see no neason why this proprietary feature is implemented. It
makes our Mozilla evangelist work more difficult: "You support BLINK, so why not
LAYERS?"

Comment 30

[Boris Zbarsky [:bzbarsky]]

Tell them, "Because CSS1 and CSS2 define text-decoration:blink as a valid
text-decoration value."

verified on Linux.

can we get a mac verification?

Comment 31

[Jacek Piskozub]

Boris: The problem is we support <blink> as well. Actually I believe support of
<blink> is dependant on support of text-decoration:blink. Blink has nothing to
do with any standard. Hence my complaint.

See bug 19258 about a proposed pref to switch <blink> off.

Comment 32

[Chase Tingley]

The patch in bug 19258 will disable all blinking text, including
|text-decoration: blink|.  <blink> itself can be disabled selectively in
userContent.css with:

blink { text-decoration: none ! important; }

so there is little reason to pref it further.

Comment 33

[John Morrison]

Verified on Mac. Thank God. Now I can design some totally cool web pages!!

Long live <BLINK>!

Comment 34

[Christine Hoffman]

Tested with 01_30_09_trunk Mozilla build. Regression - HTML 'BLINK'/CSS
'text-decoration: blink' not working. Simplified testcase attached. Reopening bug.

Comment 35

[Christine Hoffman]

Attachment: testcase to demonstrate problem

Comment 36

[Eugene Savitsky]

BTW it does not work for a while now. At least a month or more.

Comment 37

[Chase Tingley]

I suspect foul play.

Is there sufficient public outrage to warrant me fixing this regression again?

Comment 38

[Hixie (not reading bugmail)]

We should support blink. We should also have a pref to turn it off (bug 106462).
But we should support it, if just for completeness.

Comment 39

[rnorberg]

Blinking needs to be fixed - for completeness.

Comment 40

[Peter Trudelle]

->component

Comment 41

[Peter Trudelle]

->component

Comment 42

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

When did it break?

Comment 43

[John Morrison]

just because I had the old builds lying around (win2k)
  This works in:
    2001-12-01-11-trunk
    2001-12-05-09-trunk
  Does not work in:
    2001-12-10-10-trunk
    2001-12-11-06-trunk
So, somewhere between a.m. 12/05 and a.m. 12/10, this was broken.

Comment 44

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

Most suspicious checkin looks like karnaze's change to nsHTMLReflowState for bug
113424.  The full list is:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2001-12-05+08%3A00&maxdate=2001-12-10+11%3A00&cvsroot=%2Fcvsroot

Comment 45

[Bradley Baetz (:bbaetz)]

*** Bug 138765 has been marked as a duplicate of this bug. ***

Comment 46

[Hixie (not reading bugmail)]

This just got upgraded to Hixie-P1 because there is a StarGate SG1 episode that
uses Netscape 4, and the only reason appears to be that if they'd used IE the
"Access Denied" text wouldn't have been blinking impressively.

We need text-decoration: blink to be used in movies!

Comment 47

[Madhur Bhatia]

Ian, could u please provide a link to the movies that require this feature.
Thx!

Comment 48

[Chase Tingley]

Attachment: fix for the regression

Comment 49

[Chase Tingley]

dbaron was right, this was caused by the checkin that made mBlinks a bit in
mFlags -- the result of |mFlags & NS_TEXT_DECORATION_BLINK| needed to be forced
to a boolean to make the assignment give a meaningful result.

Comment 50

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

Comment on attachment 82404 [details] [diff] [review]
fix for the regression

r=dbaron.  Thanks for finding that.

Comment 51

[Marc Attinasi]

Comment on attachment 82404 [details] [diff] [review]
fix for the regression

sr=attinasi

Comment 52

[Chase Tingley]

Fixed checked in on the trunk.

I'm going to check with drivers in case this is wanted for the branch; I'll
leave this bug open until I get a response either way.

Comment 53

[Asa Dotzler [:asa]]

Comment on attachment 82404 [details] [diff] [review]
fix for the regression

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 54

[Chase Tingley]

Checked in on the branch.

Comment 55

[Hixie (not reading bugmail)]

Woohoo! StarGate SG1, here we come!
VERIFIED FIXED on trunk using attachment 41166 [details].