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)
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.
Reporter | ||
Comment 1•7 years ago
|
||
I can reproduce this problem on Mac, but not on Windows.
status-firefox57:
--- → affected
OS: Unspecified → Mac OS X
Reporter | ||
Comment 3•7 years ago
|
||
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
Reporter | ||
Comment 4•7 years ago
|
||
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
Updated•7 years ago
|
Blocks: stylo-site-issues
Assignee | ||
Comment 5•7 years ago
|
||
I... doubt that, but I can take a look on monday I guess.
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Updated•7 years ago
|
Priority: -- → P1
Comment 7•7 years ago
|
||
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)
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 8•7 years ago
|
||
Doesn't this happen by mouse click at all? I can't reproduce this, need touchpad?
Comment 9•7 years ago
|
||
I can't reproduce this with a touchpad on lenovo t470p.
Comment 10•7 years ago
|
||
Chris, is it still reproducible on macOS with the latest Nightly?
Flags: needinfo?(cpeterson)
Assignee | ||
Comment 11•7 years ago
|
||
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...
Comment 12•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
Thanks for the quick reply! I tried with disabling APZ, but no luck. :/
Reporter | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Comment 16•7 years ago
|
||
(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)
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Comment 18•7 years ago
|
||
Though I don't know much about touch events, does this still happen even if dom.w3c_touch_events.enabled is disabled?
Comment 19•7 years ago
|
||
Oh never mind. The pref is disabled by default.
Reporter | ||
Comment 20•7 years ago
|
||
(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)
Assignee | ||
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
Ok, I got it figured out, and it makes sense. Got a patch, writing a test-case.
Assignee | ||
Comment 23•7 years ago
|
||
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>
Assignee | ||
Comment 24•7 years ago
|
||
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).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(emilio)
Comment 26•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 27•7 years ago
|
||
Servo bits at https://github.com/servo/servo/pull/18390
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
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
Comment 30•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•7 years ago
|
||
Thanks Sebastian, apparently bug: <link> doesn't work inside expected: blocks. Sorry for the hassle.
Flags: needinfo?(emilio)
Comment 33•7 years ago
|
||
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
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ade99cf44de8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•