Closed Bug 89065 Opened 20 years ago Closed 19 years ago

text-decoration:blink not working

Categories

(Core :: CSS Parsing and Computation, defect)

x86
All
defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: dwueppel, Assigned: tingley)

References

()

Details

(Keywords: css1, regression, testcase, Whiteboard: [Hixie-P1][CSS1-5.4.3])

Attachments

(3 files, 4 obsolete files)

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.
Worked fine in 0.9.1 but no longer in 0.9.2
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.)
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.
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.
Assignee: clayton → pierre
Status: UNCONFIRMED → NEW
Component: HTML Element → Style System
Ever confirmed: true
Keywords: css1
QA Contact: bsharma → ian
Summary: Blink tag Not working → text-decoration:blink not working
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.
**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.
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!
Assignee: pierre → hyatt
Whiteboard: [Hixie-P3]
blinks in my html.css...still don't work.  I wan it! (just me whining)  Ignore me.
Keywords: regression
Whiteboard: [Hixie-P3] → [Hixie-P3] [ruletree]
Blocks: 91672
*** Bug 96236 has been marked as a duplicate of this bug. ***
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."
Severity: normal → enhancement
OS: Linux → All
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.
Blocks: 19258
Keywords: testcase
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.
Attached patch patch (obsolete) — Splinter Review
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?  
Keywords: patch, review
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!
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.


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.)
Attachment #48769 - Attachment is obsolete: true
Attachment #48945 - Attachment is obsolete: true
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 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.
I kept the old-style cast after a brief discussion with dbaron.  (All the cool
kids were doing it.)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
sr=hyatt
Comment on attachment 50041 [details] [diff] [review]
patch - updated as suggested by dbaron

r=dbaron
Attachment #50041 - Flags: superreview+
Attachment #50041 - Flags: review+
Fix checked in 2001-10-10 17:15 PDT.  Thanks for the patch.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Text is blinking again in 2001101103 W98. *groan*
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?"
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?
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.

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.
Verified on Mac. Thank God. Now I can design some totally cool web pages!!

Long live <BLINK>!
Status: RESOLVED → VERIFIED
Tested with 01_30_09_trunk Mozilla build. Regression - HTML 'BLINK'/CSS
'text-decoration: blink' not working. Simplified testcase attached. Reopening bug.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
BTW it does not work for a while now. At least a month or more.
I suspect foul play.

Is there sufficient public outrage to warrant me fixing this regression again?
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.
No longer blocks: 91672
Severity: enhancement → trivial
Keywords: patch, review
Whiteboard: [Hixie-P3] [ruletree] → [Hixie-P3]
Blinking needs to be fixed - for completeness.
->component
Target Milestone: mozilla1.0 → mozilla0.9.9
->component
Assignee: hyatt → dbaron
Status: REOPENED → NEW
When did it break?
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.
Whiteboard: [Hixie-P3] → [Hixie-P3][CSS1-5.4.3]
Target Milestone: mozilla0.9.9 → mozilla1.0
*** Bug 138765 has been marked as a duplicate of this bug. ***
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!
Whiteboard: [Hixie-P3][CSS1-5.4.3] → [Hixie-P1][CSS1-5.4.3]
Ian, could u please provide a link to the movies that require this feature.
Thx!
Attachment #49461 - Attachment is obsolete: true
Attachment #50041 - Attachment is obsolete: true
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 on attachment 82404 [details] [diff] [review]
fix for the regression

r=dbaron.  Thanks for finding that.
Attachment #82404 - Flags: review+
Comment on attachment 82404 [details] [diff] [review]
fix for the regression

sr=attinasi
Attachment #82404 - Flags: superreview+
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.
Assignee: dbaron → tingley
Comment on attachment 82404 [details] [diff] [review]
fix for the regression

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #82404 - Flags: approval+
Checked in on the branch.
Status: NEW → RESOLVED
Closed: 20 years ago19 years ago
Resolution: --- → FIXED
Woohoo! StarGate SG1, here we come!
VERIFIED FIXED on trunk using attachment 41166 [details].
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.