Closed Bug 19258 Opened 20 years ago Closed 18 years ago

Blinking text must be optional through a user pref

Categories

(SeaMonkey :: General, defect, P3, major)

Tracking

(Not tracked)

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: xiaotong, Assigned: tingley)

References

(Blocks 1 open bug)

Details

(Keywords: access, arch, Whiteboard: relnote-user)

Attachments

(3 files, 2 obsolete files)

This is open to address the accessibility issues according to W3C accessibility
guidelines. The priority 1 requirements for user interface:
Allow the user to turn on and off animated or blinking text.
Allow the user to run on and off animations and blinking images.
Assignee: shuang → german
don't know whether it is possible to turn off animated or blink image within
browser. german, any idea?
I don't think this is possible, but let's check with the apps team: Don?
Assignee: german → don
Severity: normal → enhancement
Target Milestone: M15
We sort of do this in 4.x anyway.

Move to M15 for later evaluation.
Summary: allow user to turn on and off animation and blinking text/image → [RFE] allow user to turn on and off animation and blinking text/image
I'm not sure how this will work with blinking images, but what if blinking text
stopped blinking when the user highlighted the text?  Just an idea.
Blocks: uaag
Animated images is covered over at bug #15145.  Don't think the others are
covered anywhere.

Chuckie, I think the idea with blinking text is that it never blinks due to
medical conditions like epilepsy.
Moving all UE/UI bugs to new component: User Interface: Design Feedback
UE/UI component will be deleted.
Component: UE/UI → User Interface: Design Feedback
Move to M16 for now ...
Target Milestone: M15 → M16
Target Milestone: M16 → Future
Keywords: access
Relnote: ~If you have epilepsy, please consider using a mozilla derivative that
does not include a gui~

Does this still require architectural changes?

Hixie, assuming !important worked, could it prevent blink?
Only by preventing all underlining, overlining and strikethrough, and it would
not be able to turn off animated images without turning _all_ of them off.
That's not really true -- we could have a separate user pref that disables
'blink' (and animation, but that's separate from CSS), since blink is optional.
I was answering the "assuming !important worked, could it prevent blink" 
question, and assuming this was referring to the user stylesheet, in which
context I stand by my statement.

However, yes, the way to fix this is indeed by using a pref and making the
code override blink and animated images based on it.
I don't believe blink and animation should be the same pref.  I wouldn't want
blink but would want animation.  Disabling animation is already fairly advanced
over at bug #17686.

As a temporary thing perhaps we could ensure blink is at a period that is known
not to effect epileptics.  I haven't heard of it being a problem in previous
browsers, I would suspect general video is a much bigger problem since it could
go a lot faster.
Keywords: relnote3
Whiteboard: relnote-user
From II Mozilla, Caput XXVII (http://www.gate.net/~shipbrk/mozilla.html):

22     And the Lizard named several of the locusts, and regarding one the Lizard
said, The <blink> tag worketh not. Whereupon the users hearing this were sore
amazed, and said they one unto another, Verily, that is no bug, but a feature to
be highly praised while it lasteth.

(seriously though... I'm surprised that the blink tag is even supported...)
Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy
Assignee: don → vishy
Chaning the qa contact on these bugs to me. MPT will be moving to the 
owner of this component shortly. I would like to thank him for all his hard 
work as he moves roles in mozilla.org...Yada, Yada, Yada...
QA Contact: claudius → zach
This bug gets my vote, and it needs to be nsCatFood too. Nobody likes blinking 
text! It needs to be optional. I want a way to disable blinking text, both in 
the BLINK element and CSS1 text-decoration: blink;. This needs to be accessible 
through the UI and fairly simple to do (clear one checkbox, for example). This 
is a serious enough user satisfaction issue to cause people to switch to a 
different browser. In fact, when I ask people why they don't use Netscape, 
BLINK is usually at the top of their list.
The proportion of pages with blinking text is minute. I can't recall the last
time I saw blinking text, in fact, and I use Mozilla on a daily basis.
That's no reason not to make it optional.
I didn't say it was; I was merely stating surprise at your statement that "in 
fact, when [you] ask people why they don't use Netscape, BLINK is usually at 
the top of their list".
Skewer:

>That's no reason not to make it optional.

I must disagree with this statement. Contributors are only able to do so 
much in such an amount of time. Every feature added and the time 
debuging those new features takes time away from fixing the really 
important bugs that we have to deal with. This alone is reason enough to 
not make it optional.
Then why even have BLINK in the first place? The time people spent writing the
code to support BLINK could have been spent writing code compatible with W3C
specifications instead of propriatery (and extremely annoying) text effects...
To make <blink> work takes one line of CSS. That's it. Nothing else. It probably
took someone around 20 seconds. That's hardly enough time to spend "writing code
compatible with W3C specifications instead of propriatery [sic] (and extremely
annoying) text effects".

Anyway, let's focus on what the bug asks for, namely, a way to disable blinking
and animations (independently). Does anyone have the time to implement this?
It was my understanding that this is a part of the accessibility effort.
I don't think this needs any extra UI, beyond that provided for other 
animations. If you want animations but not blinking text, you can edit your 
html.css. But I think most people who don't like flashy moving things will want 
to toggle both at once.

| Category:             Multimedia :::::::::: [  Normal settings :^] |
| +-------------------+                                              |
| |=Navigator=========| [/] Automatically load ima_ges and plugins   |
| |=Display===========|     [/] Allow images as _backgrounds         |
| |  Languages        | [/] Show _animations                         |
| |  Accessibility    |     [/] Allow _looping of animations         |
| |  Fonts            | [/] Play _sounds                             |
| |  Colors & Effects |     [/] Allow loo_ping of sounds             |
| |::Multimedia:::::::|                                              |

As a preliminary implementation, turning off `Allow looping of animations' 
would disable blinking text completely. As a further enhancement, turning off 
`Show animations' would make blinking text only blink three times before 
stopping.
OS: Windows NT → All
Hardware: PC → All
Errr ... Make that, `Having "Show animations" turned on, but "Allow looping of 
animations" turned off, would make blinking text blink only three times before 
stopping'. Sorry for the confusion.
I think blinking text needs its own option. Animated images are usually done
tastefully on the web, and the functionality of some websites depends on the
viewer's ability to view animated graphics. Blinking text is totally different.
It's like pestilence in my browser. It serves no purpose except to annoy.
OK, here's what we need. Right now, if we just want to disable BLINK tags, we
just have to change the entry in html.css to this:

blink {
/*  text-decoration: blink;*/
  font-weight: bold;
}

However, anyone who uses the CSS text-decoration: blink; will still be able to
create this irritating effect. Adding UI to change html.css would be only a
partial solution, we need UI to disable text-decoration: blink; (and therefore,
a corresponding entry in prefs.js).
Attached file Blinking text testcase
Hey, looks like someone's ahead of us. Blinking text appears to be disabled in
2001061504 Win98.
Adding fcc508 keyword.

This is needed for FCC Section 508 compliance.
Without this option, Mozilla based products cannot be sold to US Federal
Government agencies.
Severity: enhancement → major
Keywords: fcc508
This appears to be fixed to me. Animated images can be set to loop normally,
once, or never in the Privacy and Security > Images pref. Blinking text doesn't
blink anymore. Embedded sounds are handled by plugins.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
On one hand, I'm glad we have the animation pref now.

However, how is a user supposed to find animation prefs under "Privacy and
Security". What do they have to do with that?

IMO, no one looking for animation or image prefs would look there - they would
look under "Appearance" prefs or something.
aaron, that's covered in another bug
Someone should open a bug for disabling 'blink' (the fact that it does not work
right now is a bug, and it will work eventually).
If bug 89065 is fixed, this bug will be reopened with this summary. There's not
much chance we'll lose the animation pref now, and this bug already has the
votes, testcase, keywords, and comments.
Keywords: nsCatFood
Summary: [RFE] allow user to turn on and off animation and blinking text/image → Blinking text must be optional through a user pref
And this is a 4xp if you consider the

user_pref("browser.blink_allowed", false);

workaround which worked in older Netscape 4 builds (but for some reason doesn't
work in the latest NS4 build).
Keywords: 4xp
This bug isn't "fixed", there's no user pref.  Reopening and adding dependency on 
bug 89065.
Status: RESOLVED → REOPENED
Depends on: 89065
Resolution: FIXED → ---
Taking this; I'm an inch away from fixing bug 89065, so I owe it to the world to
fix this bug as well.
Assignee: vishy → tingley
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Keywords: helpwanted
Target Milestone: Future → mozilla1.0
This adds a 4.x-esque "browser.blink_allowed" pref, which defaults to true.  I
added it to the prefs to which nsPresContext listens so that it would be easily
accessible when building the reflow state...

The patch doesn't do anything about a UI, including tying this to animation
looping.  mpt told me on #mozilla that he still believes in this, but given the
current state of the animation loop UI, I don't understand how that's intuitive
at all:

+ Animated images should loop ---------------+
| (*) As many times as the image specifies   |
| ( ) Once                                   |
| ( ) Never                                  |
+--------------------------------------------+

At any rate, there are two separate things going on.  I'd like to try to get
reviews on a pref-only fix now to limit the blink-inflicted suffering on the
trunk, while discussion continues about the UI issue.

cc'ing ftang and attinasi, who seem (from the cvs logs) to know this code well.
Keywords: patch, review
I think the best existing pref panel to put this on is Appearance/Fonts, as a
checkbox:

[*] Allow documents to use other fonts
[ ] Allow documents to display blinking text

I can't say that I agree with the decision to make blinking text enabled by
default, but I suppose if it wasn't, nobody would ever enable it.

By the way, it would be a good idea to tweak the blink rate so blinking text is
* easier to read
* has less of a headache-inducing strobe effect
For example, it could be on for three seconds, then off for half a second. But
that is a distant second choice to adding a pref.
I think you are better off defaulting the value of the pref in the code and NOT
adding the pref to the all.js file. Simply remove the pref from all.js, and in
the presShell code initialize boolPref to PR_TRUE before calling GetBoolPref,
and then you can ignore the result of that call too. We should not have to add
default values to the pref file.

Comment on attachment 53052 [details] [diff] [review]
patch - support for "browser.blink_allowed"

Good idea.
Attachment #53052 - Attachment is obsolete: true
Comment on attachment 53197 [details] [diff] [review]
v2 -- assume a default value instead of adding to all.js

sr=attinas
Attachment #53197 - Flags: superreview+
Since this is important for a lot of reasons (accessibility to users with
epilepsy, FCC requirements), I think we need to get a front-end built to make
the choice available to an end-user. Organizations which would rather not have
blinking text won't go to the trouble to edit JS config files to remove it - not
when IE doesn't use it at all. This may be something that needs to be moved to a
new bug, but keep it in mind.
Keywords: nsenterprise
I seem to recall that it was preferred that there always be a default in all.js,
although I could be wrong.

Why does the pres context need to know about this pref?  Can't it all be handled
from a single static prefchange callback in the reflow state and two static
variables (an initialized boolean, and a blink pref boolean)?  Wouldn't that be
preferable? (It avoids the virtual function call and avoids increasing the size
of the pres context.)  But maybe I'm being silly...  Do we handle most other
similar prefs in the pres context?
I think it was put in the PresContext because we have several similar prefs
there (page and link colors, for example). Maybe they started out there because
the presContext is so widely available to layout? I agree that it is worth
trying to move the prefs out of there - the PresContext is turning into a
dumping grounds. I think that should be another bug though, one which I will
open if you agree.
I'm open to either solution -- there's no reason why a single static callback
wouldn't suffice; I was following the example of the link color, etc, prefs
already in the pres context (as you suggested).  I can clean this up now to save
some work later and avoid bloat.

Also, the more I ask around the more it sounds like dbaron is right about
including defaults in all.js.

Writing this one once more would also let me clean up that bogus logic with the
NS_FAILED call, which has been bugging me (should default to true, ignore rv).

Attinasi, any objections?  

Pulling this in; I'd like to get this into 0.9.6 on humanitarian grounds.
Target Milestone: mozilla1.0 → mozilla0.9.6
No objects from me. I am curious why we want default values in all.js - won;t
that just make the file larger (a minus for distribution) and slower to load (a
minus for performance)? I guess it would be nice to see all the values that have
meaning (a plus for usability).
I thought storing default values was the whole point of all.js and friends.  (As
well as being good documentation -- the only documentation -- for what the
default values are.)  If not that, then what's it for?  If we have performance
problems in reading lines from all.js, then we're in trouble anyway and ought to
rethink our prefs strategy.  But it doesn't seem to be showing up as a problem
on profiling runs.
Comment on attachment 53197 [details] [diff] [review]
v2 -- assume a default value instead of adding to all.js

bam!
Attachment #53197 - Attachment is obsolete: true
I thought all.js and friends were for setting values *other than* the defaults,
such that they apply to all profiles. 

Besides the documentation benefits, what is the motivation to put all default
values in the pref file? Even if there is not performance problem reading large
pref files, it cannot be any faster than reading small ones! And we *do* have a
performance problem in loading - it may not be due to the prefs, but anything
done at load time contributes to how ong it takes to get to your first webpage...

BUT - I don't really care too much here - if everybody thinks we should have
default values in all.js that is cool with me, I just would like to know why.
And I apologize for cluttering this bug with this tangential issue.
The default pref is back in all.js by popular demand.  The code has been moved
entirely into nsHTMLReflowState.cpp and rewritten to use a static callback and
associated state.
Wouldn't it be more logical to write:

  mBlinks = (parentReflowState && parentReflowState->mBlinks);
  if (!mBlinks && BlinkIsAllowed()) {

?  After all, the pref won't change during a single reflow.  Other than that, 
r=dbaron
I wonder why there seems to be little use of the helpful
nsPresContext::GetCachedBoolPref(). It seems appropriate here.
Why should we waste time registering and unregistering callbacks for each pres
context created and destroyed when we want to keep the pref at one place in
static data and can always know what it is?
If that's what you intended then the patch does the job. Forget my comments.
...And BTW, for the record, your question applies equally well to the other
prefs in the pres context... Possible gain in re-thinking the matter over there.
Perhaps, GetCachedBoolPref() and friends could just be wrappers around static
functions.
Comment on attachment 54322 [details] [diff] [review]
v4 - logic switch for dbaron

sr=attinasi
Attachment #54322 - Flags: superreview+
attinasi, tingley: whoa there... 

For consistency with other prefs, surely this should be called
"browser.blink.enabled" ? This is not just an academic point; we want to keep
the pref namespace tidy and consistent.

Gerv
Gerv, I saw consistency:

 pref("browser.anchor_color",                "#0000EE");
 pref("browser.visited_color",               "#551A8B");
 pref("browser.underline_anchors",           true);
+pref("browser.blink_allowed",               true);

How is 'browser.blink.enabled' any more consistent? To be honest I could go
either way, but the current pref looks fine to me, in keeping with similar pref
strings.
I chose the pref name based on claims that it was what NS4 used (see comment
from  Skewer 2001-08-26 13:58).  

I have no particular attachment to this name; I'll change it if given a good
candidate and a compelling reason.
I looked through all.js and it seems that most prefs of the "on/off" sort end
with ".enabled". But if the current pref is 4xp, that's slightly different. It's
probably not bothering with.

Gerv
I'll check this in today then.
Checking in nsHTMLReflowState.cpp;
/cvsroot/mozilla/layout/html/base/src/nsHTMLReflowState.cpp,v  <-- 
nsHTMLRefloState.cpp
new revision: 1.122; previous revision: 1.121
done

Checking in all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.289; previous revision: 3.288
done

Backend changes checked into trunk. Thanks Chase!
Since at the most basic level, the accessibility requirement has been meant, I'm
going to mark this bug fixed.  There's the still the issue of UI control over
the pref -- I've spun off bug 106462 to deal with this.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Verified 2001102403 W98 using attachment 38667 [details].
*** Bug 172897 has been marked as a duplicate of this bug. ***
Component: User Interface Design → Browser-General
*** Bug 190769 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.