188761 - Change visible:invisible times in <blink> (make it less evil)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P2)

Description

[Bartosz Wucke]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212

The worst problem with the <blink> tag is that the text disappears for
considerable amount of time, when you can't read it, and you lose track of the
point where you finished reading - before you find it anew, the text disappears
again. Bug #173540 suggests a pretty-looking solution, but it would probably
require much more computing power, quite a bit of code and I'm afraid the effect
wouldn't cause the blinking text stand out enough when it comes to important things.

My solution is much simpler - just make the text be visible most of the time and
disappear for a really short moment. Now it is visible for about half a second
and invisible for another half. Make it visible for 0.8s and invisible for 0.2s
(or something like this - a few experiments what is sufficient would help) -
This way the blinking still stands out strongly, but reading isn't so unpleasant
- you don't actually wait for the text to reappear and don't look for "lost
thread". From coding side this should require just changing parameters in
existing loops, without adding any new code.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

> From coding side this should require just changing parameters in
> existing loops, without adding any new code.

Nope.  Since there are no loops.  There is just a timer that fires at 750 ms
intervals and flips the text on and off.  So it would either need to use
something other than a boolean for the blink state (eg values 0-4, text off for
0, on for the others, with the timer firing a lot more often) or you'd need to
write two separate timers and synch them somehow...

Fwiw, I think the idea of having the timer take on values in Z/n for some n,
having the timer fire every 1/n seconds and making sure to paint only when the
text actually needs to go off or come on may work...

Comment 2

[Hixie (not reading bugmail)]

I read somewhere that the US military standard for blinking was 3 cycles on for
every one cycle off. Someone should do some research into what the best timings
are to avoid seizures, etc.

Google didn't help me much.

Comment 3

[Jo Hermans]

According to Jamie Zawinski :
http://groups.google.com/groups?selm=31C46266.500F%40netscape.com&oe=UTF-8

I don't know if it's true, but it's funny at least :-)

Comment 4

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

Taking

Comment 5

[Robert Morris]

Since 1.4a is past, should we up the target milestone?

By the way, why does Netscape support BLINK anyway? I though it was a NS 4-only
proprietary extension to HTML, not a standard. I'm still searching Bugzilla for
that one (feel free to point me to the bug if it exists). :-)

Comment 6

[Brendan Eich [:brendan]]

BLINK has been around since Netscape 1.

/be

Comment 7

[Bill Mason]

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

Comment 8

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

Attachment: testcase

Comment 9

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

FWIW, my tests in Netscape 4 (for Linux) and Netscape 3 (for Mac) show that the
timing there is 1/2 on, 1/2 off.

That agrees with the code in classic:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/lib/layout/laylayer.c&rev=3.9&mark=72,119-137,175-181#71

Comment 10

[Bartosz Wucke]

Wouldn't something like this do the trick?
(750:250 give the 3:1 military ratio. According to this, and JZW's comment
(referenced in comment #3) this bug becomes "normal", not RFE.)

#define LO_BLINK_RATE_SHOWN 750
#define LO_BLINK_RATE_HIDDEN 250

static void
lo_blink_callback(void *closure)
{
    MWContext *context = (MWContext *)closure;
               
    if (context->blink_hidden == FALSE) {
       CL_HideLayerGroup(context->compositor, LO_BLINK_GROUP_NAME);
       context->blink_hidden = TRUE;
       context->blink_timeout = FE_SetTimeout(lo_blink_callback,
                                                (void *)context,
                                                LO_BLINK_RATE_HIDDEN);
    } else {
       CL_UnhideLayerGroup(context->compositor, LO_BLINK_GROUP_NAME);
       context->blink_hidden = FALSE;
       context->blink_timeout = FE_SetTimeout(lo_blink_callback,
                                                (void *)context,
                                                 LO_BLINK_RATE_SHOWN);
   }
               
}

Comment 11

[Jamie Zawinski]

> FWIW, my tests in Netscape 4 (for Linux) and Netscape 3 (for Mac) show
> that the timing there is 1/2 on, 1/2 off.

I guess that means it's been broken longer than I thought (damn you Aleks!)
NS3 cmd/xfe/lay.c line 2666:

    /* Blinking.  Oh the humanity. */

    static int blink_state = 0;
    static int blink_time = 250;
    ...

    static void
    fe_blink_timer (XtPointer closure, XtIntervalId *id)
    {
    ...
          fe_display_text (context, 0, b->text, 0, b->text->text_len,
                           (blink_state == 3));
    ...
      blink_state = (blink_state + 1) % 4;
    ...

Comment 12

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

Attachment: patch to do 750 on, 250 off

Comment 13

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

Comment on attachment 128457 [details] [diff] [review]
patch to do 750 on, 250 off

>+  sState = (sState + 1) % 4;
>+  if (sState == 1 || sState == 2)
>+    // States 0, 1, and 2 are all the same.
>+    return;

Changed to |return NS_OK;| in my tree.	I wish gcc would make that an error
like many other compilers...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128457 [details] [diff] [review]
patch to do 750 on, 250 off

Looks good.

Comment 15

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

Fix checked in to trunk, 2003-07-24 14:26 -0700.

Comment 16

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

And modification, at brendan's suggestion (use unsigned type) checked in as
well.  Which also fixes the problem that caillon pointed out that I forgot to
change one of the PRBool's to PRInt32.  Oops.

Comment 17

[Brendan Eich [:brendan]]

It's important to strength-reduce (x % power_of_two_constant) by hand via &, or
else make x have unsigned integer type and use a non-broken compiler.

Otherwise a sign test is required before the optimizer can emit the
strength-reduced code, and the negative case requires subtracting the modulus
from the & result (unless the compiler can analyze data flow and determine that
x is always non-negative).

/be