Closed Bug 1393861 Opened 7 years ago Closed 7 years ago

stylo: switching Slack channels sometimes requires multiple clicks

Categories

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

57 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

STR:
1. Open Slack in Nightly, such as https://mozilla.slack.com/
2. Try to switch channels by clicking channel names in the list.

RESULT:
At least half the time, you will need to click two or three times before the channel will switch.

Disabling layout.css.servo.enabled seems to fix the problem, but it's hard to be sure because the problem is intermittent.

I noticed this problem today. Mossop said he seen this problem for a couple days.
I can reproduce this problem on Mac, but not on Windows.
OS: Unspecified → Mac OS X
I'm on Windows.
OS: Mac OS X → All
The problem only happens when I try to switch channels by tap my MacBook Pro's touchpad, but not when press-click the touchpad.

This doesn't seem to be a recent regression. I can reproduce on both Nightly 57 and Beta 56. Since the problem is somewhat intermittent, bisecting was difficult. I think the regression might be in this changeset range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a4490a86f11f183c48d603340d23725abe6d750c&tochange=1023371e0802e3a22cfac58f1f7f3968a6112cfe
Version: 51 Branch → 57 Branch
I have further narrowed the possible regression range:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c116f4807f26e657712bdf97f90ab09c51d0fb7f&tochange=34fe793e4c4db9813343589ad9fc4a7f0f8883a7

@ Emilio or Cameron, could this Slack channel switching bug be a regression from bug 1379505 or bug 1376964? I suspect bug 1379505 because 1376964 landed and was backed out in this same range.

https://github.com/servo/servo/pull/17688
Blocks: 1379505
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
I... doubt that, but I can take a look on monday I guess.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> I... doubt that, but I can take a look on monday I guess.

I guess I can rephrase this as "I'd be surprised it's that change since what we were doing before was pretty broken for a variety of reasons". But I guess it can technically cause this, so I'll investigate this.
Priority: -- → P1
Emilio said he was going to look at this. This is our only outstanding site issue at the moment, so it'd be good to get it nailed down.
Assignee: nobody → emilio
Flags: needinfo?(cam)
Status: NEW → ASSIGNED
Doesn't this happen by mouse click at all? I can't reproduce this, need touchpad?
I can't reproduce this with a touchpad on lenovo t470p.
Chris, is it still reproducible on macOS with the latest Nightly?
Flags: needinfo?(cpeterson)
Yeah, I can't repro anymore on Linux, but a coworker just tested and can reproduce it on OSX, but only when tapping on the trackpad... I'm not sure how can I debug this, I have no OSX machine, and it seems kinda spurious...
Masayuki, do you know what the difference between tapping on touchpad and clicking mouse button in our implementations is? Also difference on OSX?
Flags: needinfo?(masayuki)
I'm not sure. How about enabling/disabling APZ?
Flags: needinfo?(masayuki)
Thanks for the quick reply!  I tried with disabling APZ, but no luck. :/
I can still reproduce on today's Nightly build on macOS when tapping my MacBook Pro's touchpad. Clicking the touchpad switches channels works correctly. Tapping the touchpad in Chrome works correctly.

I have only seen this problem when trying to switch channels on Slack. For example, tapping the touchpad can switch channels in irccloud client works correctly.
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson [:cpeterson] from comment #15)
> I can still reproduce on today's Nightly build on macOS when tapping my
> MacBook Pro's touchpad. Clicking the touchpad switches channels works
> correctly. Tapping the touchpad in Chrome works correctly.
> 
> I have only seen this problem when trying to switch channels on Slack. For
> example, tapping the touchpad can switch channels in irccloud client works
> correctly.

Chris, given the problem is obvious when tapping (at least on my coworker's macbook), and you seemed to not be that confident at the regression range in comment 3, could we try to narrow it down?

It doesn't help if the suspect ends up being bug 1379505, but it'd help a lot if it ends up _not_ being so. I can try to debug tomorrow further with Andrei.

Thanks!
Flags: needinfo?(cpeterson)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> It doesn't help if the suspect ends up being bug 1379505, but it'd help a
> lot if it ends up _not_ being so. I can try to debug tomorrow further with
> Andrei.

I guess since the other suspect bug really was backed out, it's pretty likely it was it. I have an hypothesis on what may be the problem, but confirming that's the suspect bug would be really helpful anyway.
Though I don't know much about touch events, does this still happen even if dom.w3c_touch_events.enabled is disabled?
Oh never mind.  The pref is disabled by default.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Chris, given the problem is obvious when tapping (at least on my coworker's
> macbook), and you seemed to not be that confident at the regression range in
> comment 3, could we try to narrow it down?

Emilio, I bisected the regression again and found the same regression range as comment 4, which points to your bug 1379505:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c116f4807f26e657712bdf97f90ab09c51d0fb7f&tochange=34fe793e4c4db9813343589ad9fc4a7f0f8883a7

I see the regression range includes WebRender bug 1377571, but I don't think it's related because I don't have WebRender enabled and this Slack bug goes away if I pref off Stylo.
Flags: needinfo?(cpeterson)
So, the cool part is that I figured out something that affects it.

In particular, if you remove the following rule:

.p-channel_sidebar__channel::before {
    content: '\E125';
}

It suddenly works again.

So presumably we're reframing something unnecessarily, and that makes events to be lost? That reminds me a lot to bug 1376406.

I'm still building on the loaner, but I think I have an idea of why this can happen.
See Also: → 1376406
Ok, I got it figured out, and it makes sense. Got a patch, writing a test-case.
So here's a test-case that fails: This should alert "block" two times.

<!DOCTYPE html>
<style>
  #test {
    display: flex;
  }
  #test::before, #test::after {
    content: "test";
    display: inline-block;
    color: green;
    transition: color 1s ease;
  }
  #test.restyled::before {
    color: blue;
  }
</style>
<div id="test">
</div>
<script>
onload = () => {
  document.body.offsetTop;
  alert(getComputedStyle(test, "::before").display);
  alert(getComputedStyle(test, "::after").display);
}
</script>
Now, about why the bug didn't repro with my patch, is because I got rid of the "grab the style from the parent" optimization (and instead we moved that to the frame constructor). That means that we computed the style without fixup, and when diffing it with the old style, since the display changed, we reframe it.

I'll write a test harness test, and I'll also write a mochitest to ensure we don't reframe in this situation (which is a consequence of the bug).
Flags: needinfo?(emilio)
Comment on attachment 8904574 [details]
Bug 1393861: Correctly apply the display fixup for ::before and ::after pseudo-elements.

https://reviewboard.mozilla.org/r/176418/#review181618
Attachment #8904574 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1caa4e7a8319
Correctly apply the display fixup for ::before and ::after pseudo-elements. r=heycam
Backed out for busting web-platform-tests:

https://hg.mozilla.org/integration/autoland/rev/cccd53c58f081e6fa08a233bbd12fe036f2e5d80

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1caa4e7a8319bd5fea0a98faa4a48713dca40770&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128880517&repo=autoland

LOG ERROR | Traceback (most recent call last): File "Z:\task_1504697948\build\tests\web-platform\tests\tools\wptrunner\wptrunner\testrunner.py", line 350, in run new_state = self.wait_event() File "Z:\task_1504697948\build\tests\web-platform\tests\tools\wptrunner\wptrunner\testrunner.py", line 424, in wait_event return f(*data) File "Z:\task_1504697948\build\tests\web-platform\tests\tools\wptrunner\wptrunner\testrunner.py", line 547, in test_ended stack=result.stack) File "Z:\task_1504697948\build\venv\lib\site-packages\mozlog\logtypes.py", line 49, in inner data = converter.convert(*args, **kwargs) File "Z:\task_1504697948\build\venv\lib\site-packages\mozlog\logtypes.py", line 90, in convert out_value = self.args[key](value) File "Z:\task_1504697948\build\venv\lib\site-packages\mozlog\logtypes.py", line 125, in __call__ (value, type(value).__name__, self.name, self.__class__.__name__)) ValueError: Failed to convert value bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1396844 of type unicode for field expected to type SubStatus
Flags: needinfo?(emilio)
Thanks Sebastian, apparently bug: <link> doesn't work inside expected: blocks. Sorry for the hassle.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ade99cf44de8
Correctly apply the display fixup for ::before and ::after pseudo-elements. r=heycam
https://hg.mozilla.org/mozilla-central/rev/ade99cf44de8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.