text-decoration:blink not working

VERIFIED FIXED in mozilla1.0

Status

()

defect
--
trivial
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: dwueppel, Assigned: tingley)

Tracking

({css1, regression, testcase})

Trunk
mozilla1.0
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P1][CSS1-5.4.3], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
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

Comment 5

18 years ago
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]

Comment 9

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

Updated

18 years ago
Keywords: regression
Whiteboard: [Hixie-P3] → [Hixie-P3] [ruletree]

Updated

18 years ago
Blocks: 91672
(Assignee)

Comment 10

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

Comment 11

18 years ago
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

Comment 12

18 years ago
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
(Assignee)

Comment 13

18 years ago
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.
(Assignee)

Comment 14

18 years ago
Posted patch patch (obsolete) — Splinter Review
(Assignee)

Comment 15

18 years ago
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!
(Assignee)

Comment 17

18 years ago
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.


(Assignee)

Comment 18

18 years ago
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.)
(Assignee)

Updated

18 years ago
Attachment #48769 - Attachment is obsolete: true
(Assignee)

Updated

18 years ago
Attachment #48945 - Attachment is obsolete: true
(Assignee)

Comment 21

18 years ago
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.
(Assignee)

Comment 23

18 years ago
(Assignee)

Comment 24

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

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

Comment 25

18 years ago
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
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 28

18 years ago
Text is blinking again in 2001101103 W98. *groan*

Comment 29

18 years ago
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?

Comment 31

18 years ago
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.

(Assignee)

Comment 32

18 years ago
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

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

Long live <BLINK>!
Status: RESOLVED → VERIFIED

Comment 34

18 years ago
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 → ---

Comment 36

18 years ago
BTW it does not work for a while now. At least a month or more.
(Assignee)

Comment 37

18 years ago
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]

Comment 39

18 years ago
Blinking needs to be fixed - for completeness.

Comment 40

17 years ago
->component
Target Milestone: mozilla1.0 → mozilla0.9.9

Comment 41

17 years ago
->component
Assignee: hyatt → dbaron
Status: REOPENED → NEW

Comment 43

17 years ago
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]

Comment 47

17 years ago
Ian, could u please provide a link to the movies that require this feature.
Thx!
(Assignee)

Comment 48

17 years ago
Attachment #49461 - Attachment is obsolete: true
Attachment #50041 - Attachment is obsolete: true
(Assignee)

Comment 49

17 years ago
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 51

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

sr=attinasi
Attachment #82404 - Flags: superreview+
(Assignee)

Comment 52

17 years ago
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+
(Assignee)

Comment 54

17 years ago
Checked in on the branch.
Status: NEW → RESOLVED
Last Resolved: 18 years ago17 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.