1385831 - stylo: site issue: links are unclickable if you clicked on one on mcfit.com

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Darkspirit]

Attachment: 2017-07-31_12-20-33.mp4

Nightly 56 x64 20170730100307 @ Debian Testing

STR (see video):
1. https://www.mcfit.com/de/
2a. If you have Basic Tracking Protection enabled, you can't click on any link/button
2b. Click on a link or button.
3. hover links/buttons, nothing happens, you can't click on them

As I haven't clicked on every link, I can't say "Visited links are unclickable" in the summary.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Huh, interesting.

So you can't do anything because there's a huge <div> taking over the page.

That page is a bit |display: table| thing, so even though I'm not sure yet about what's going wrong, I think I have an idea...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Err, I meant that that <div> is a big display: table thing we probably failed to reframe properly.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Hmm... We're getting _so_ confused.

The page has some CSS that reads like:

.on-loading-complete .mod-load-indicator--mask{opacity:0;visibility:hidden;transition:300ms 500ms}.mod-flexbox{display:-webkit-flex;display:-ms-flexbox;display:flex;-webkit-flex-direction:row;-moz-flex-direction:row;-ms-flex-direction:row;flex-direction:row;-webkit-flex-wrap:wrap;-ms-flex-wrap:wrap;flex-wrap:wrap;margin-left:-10px;margin-right:-10px}

We're styling the |.on-loading-complete .mod-load-indicator--mask|, but somehow applying the .mod-flexbox rule... wtf.

Comment 4

[Jeremy Chen ([e-mail])]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Err, I meant that that <div> is a big display: table thing we probably
> failed to reframe properly.

Yeah, I can see the huge <div> with z-index: 999 blocking the links in the latest nightly Stylo as well.

Supposely the .on-loading-complete rule should be applied after finishing loading, and a { visibility: hidden; } should be applied to the huge <div> accordingly. I haven't managed to make a reduced testcase.... will keep working on making a reduced testcase tomorrow.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Hmm... We're getting _so_ confused.
> 
> The page has some CSS that reads like:
> 
> .on-loading-complete
> .mod-load-indicator--mask{opacity:0;visibility:hidden;transition:300ms
> 500ms}.mod-flexbox{display:-webkit-flex;display:-ms-flexbox;display:flex;-
> webkit-flex-direction:row;-moz-flex-direction:row;-ms-flex-direction:row;
> flex-direction:row;-webkit-flex-wrap:wrap;-ms-flex-wrap:wrap;flex-wrap:wrap;
> margin-left:-10px;margin-right:-10px}
> 
> We're styling the |.on-loading-complete .mod-load-indicator--mask|, but
> somehow applying the .mod-flexbox rule... wtf.

So the issue I was seeing here seems to be a devtools bug... The mod-load-indicator--mask does have display: table, and _does_ have visibility: hidden. So it sounds like we're missing a change-hint here or there, since reframing it does fix the issue.

I suspect this is again the missing restyling of table anon-boxes, but I'll try to reduce it as well.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Seems like they just add the on-loading-complete class to the body when the window onload event fires, so in theory it shouldn't be hard to make a reduced test-case... Let me try real quick.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: reduced test-case.

I'm pretty sure this is because we're failing to restyle the anonymous cells that get created for the table. If I change the display to something that doesn't generate anonymous boxes (like display: block), I get the correct behavior back.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: test-case with the same frame tree, but working

So this is instead because of the anonymous wrapper stuff. We need to get that fixed ASAP.

I'll talk with Boris in order to discuss what the best way to fix it is. I tried today one of the ways we discussed previously, but it's fairly non-trivial :-)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed by bug 1388625.

Comment 11

[Darkspirit]

Verified fixed in Nightly 57 x64 20170812100345 @ Debian Testing. Thank you!

Comment 12

[Darkspirit]

(This site is still unusable with Beta. Uplift? Or do you accept such bugs for Beta as long they are no crashes?)

Comment 13

[Bobby Holley (:bholley)]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #12)
> (This site is still unusable with Beta. Uplift? Or do you accept such bugs
> for Beta as long they are no crashes?)

Hm, tough. In general I think we're mostly just interested in stability metrics out of the beta experiment. On the other hand, if the lack of bug 1388625 is significant enough to make the site unusable, then we may want to uplift.

Cameron, given that bz is out, what do you think? Should we uplift bug 1388625? If so, can you nominate? Are there any other things that we'd need to uplift as well?

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> Hm, tough. In general I think we're mostly just interested in stability
> metrics out of the beta experiment. On the other hand, if the lack of bug
> 1388625 is significant enough to make the site unusable, then we may want to
> uplift.

My general feeling is that if the beta experiment is really about stability metrics, and if mcfit.com isn't a major site and we haven't seen any other reports of sites being similarly unusable, that we shouldn't uplift it.  Also I just tried rebasing that bug's patches on beta, and I get a panic.  I tried with bug 1368290 included too, which I thought might be the other dependency, but it still panics.  I'm not sure it's worth putting in time to work out what's going on.

Comment 15

[Darkspirit]

(In reply to Cameron McCormack (:heycam) from comment #14)
> if mcfit.com isn't a major site
It's the most popular fitness studio in Germany. But I haven't seen this bug elsewhere.
Poor german Beta users who thinks they test the next stable. xD

> I'm not sure it's worth putting in time to work out what's going on.
Fx 57 is more important then, I think.

Comment 16

[Darkspirit]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #12)
> Or do you accept such bugs for Beta as long they are no crashes?
*don't you

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> Hm, tough. In general I think we're mostly just interested in stability metrics out of the beta experiment.
Ok, thanks for clarifying!

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Note that this bug also affected outlook mail... So there's probably a fair amount of people that would notice it...

Comment 18

[Chris Peterson [:cpeterson]]

Outlook Mail is pretty popular. live.com is Alexa rank #15. But given that the uplift panics and will require more debugging, I don't think we should spend any more time trying to uplift this fix.