Remove autoscroll property from UI reducer

VERIFIED FIXED in Firefox 55

Status

enhancement
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [console-html])

Attachments

(1 attachment, 1 obsolete attachment)

This is a follow up for bug 1367266

The current implementation of "Scroll to bottom" feature isn't ideal since it assumes that MESSAGE_ADD is the last fired action. 

See this comment:
https://dxr.mozilla.org/mozilla-central/rev/f7adbf457ee20eeffde72694e0d17d73616e3cfd/devtools/client/webconsole/new-console-output/reducers/ui.js#23

This solution tends to break as new actions/features are introduced and we should remove the "autoscroll" state altogether.

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [console-html]
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: iulia.cristescu
Attachment #8871695 - Attachment is obsolete: true
Comment on attachment 8872285 [details]
Bug 1368022 - Remove autoscroll property from UI reducer;

https://reviewboard.mozilla.org/r/143788/#review147472

Looks good and feel better :)
Since you're dealing with the scrolling, do you think you could add some mocha tests to make sure we only set `shouldScrollBottom` when adding a message, opening a group, clearing a filter, ... 
I don't know if it's doable with mocha only though.
There is a bug for this : https://bugzilla.mozilla.org/show_bug.cgi?id=1307934 , so feel free to do it here or there
Attachment #8872285 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8872285 [details]
> Bug 1368022 - Remove autoscroll property from UI reducer;
> 
> https://reviewboard.mozilla.org/r/143788/#review147472
> 
> Looks good and feel better :)
Thanks Nicolas for the review!

> Since you're dealing with the scrolling, do you think you could add some
> mocha tests to make sure we only set `shouldScrollBottom` when adding a
> message, opening a group, clearing a filter, ... 
> I don't know if it's doable with mocha only though.
> There is a bug for this :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1307934 , so feel free to do it
> here or there
Let's do it in that bug (it seems to be dedicated to a scrolling test)

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b9687c90aea
Remove autoscroll property from UI reducer; r=nchevobbe
Backed out for eslint failures, e.g. in console-output.js:

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

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7b9687c90aea55f7893ecfb0ccd5f0c954e36eb0&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=102842919&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/webconsole/new-console-output/components/console-output.js:50:8 | Expected indentation of 6 spaces but found 7. (indent)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/webconsole/new-console-output/reducers/ui.js:10:3 | 'MESSAGE_ADD' is assigned a value but never used. (no-unused-vars)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/webconsole/new-console-output/reducers/ui.js:11:3 | 'REMOVED_MESSAGES_CLEAR' is assigned a value but never used. (no-unused-vars)
Flags: needinfo?(odvarko)
Eslint errors fixed, thanks!

Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61904b6d9ab4
Remove autoscroll property from UI reducer; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/61904b6d9ab4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: 55.6 - May 29 → 55.7 - Jun 12
I can confirm the initial issue is fixed on 55.0a1 (2017-06-11) using  Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.