Opened 11 years ago
Closed 11 years ago
#10623 closed Bug (fixed)
[Webkit] Page is scrolled when opening dropdown
Reported by: | Piotrek Koszuliński | Owned by: | Piotrek Koszuliński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.2.1 |
Component: | General | Version: | 4.1.2 |
Keywords: | Webkit | Cc: |
Description
- Open some long sample to have a scrollbar (e.g.
samples/datafiltering.html
). - Scroll few px down.
- Open e.g. a font-size dropdown.
- Page was scrolled.
First bad commit: git:d0f600ed.
Attachments (1)
Change History (12)
comment:1 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 11 years ago by
Owner: | set to Olek Nowodziński |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 11 years ago by
comment:5 follow-up: 6 Changed 11 years ago by
I gave this issue a try and I don't have good news :). I found that focusing this.element
(https://github.com/ckeditor/ckeditor-dev/blob/d0f600ed461cc3d3fbe839113de5e7080a5bb412/plugins/listblock/plugin.js#L223) at any moment will always cause scroll. I even removed that statement completely and focused it later from console and the same happened.
Webkit/Blink seems to scroll the viewport to fit as much of the this.element
(which is a div inside panel's iframe) as it can. Scrolling happens most often when opening the styles drop down because it's the longest (so that div element is the highest) - in fact it does not fit in the viewport completely.
So if we need to focus the list element (and I assume we do), we need to find out what are the ways to avoid scrolling when focusing (e.g. we can set the height to 0px?) or we can restore the scroll position as we already do e.g. when focusing pastebin. In fact the second option may be the most stable solution, but not perfect, since not only viewport may be scrolled by the browser.
PS. I've read in #10361 that there were problems with stability and Olek mentions that too. Looking at this code: https://github.com/ckeditor/ckeditor-dev/blob/d0f600ed461cc3d3fbe839113de5e7080a5bb412/plugins/listblock/plugin.js#L223-L228 I don't understand why selected
is focused with delay, but this.element
is focused immediately. They are in the same frame and selected is a descendant of this.element
, so they may be no reason to differentiate this.
Second thing is that it would be worth checking higher timeouts. I often see differences between tested machines (even when using the same browser) which are caused by a different order of events which is happens because of number of micro timeouts (which theoretically are predictable) and DOM events (which completely aren't predictable).
comment:6 Changed 11 years ago by
@Reinmar: I spent some time investigating this issue and playing with setTimeout
brings nothing more but... more timeout. It's not entirely the timeout thing what we need, I guess. Something else, like the pastebin focus solution could work, but I'd look for simpler solution first.
Still, apart from the UX aspect, there's a11y that makes me worry as we're unable to verify it. I'm eager to see Fred dealing with that first.
comment:7 Changed 11 years ago by
Owner: | changed from Olek Nowodziński to Frederico Caldeira Knabben |
---|---|
Status: | assigned → review |
I didn't find any good solution for this as well. Any attempt to make it work breaks the a11y side of it.
I've just pushed t/10623, which contains one of the ideas we had on #10361. I'm not a big fan of it, but it can be the only solution for now.
comment:8 Changed 11 years ago by
This issue (scrolling) is not critical, so if we cannot quickly find a decent solution, I'd rather postpone it than include insecure hack.
I pushed t/10623b with a simple hack that restores viewport scroll position after focusing element. Isn't this enough?
comment:9 Changed 11 years ago by
Owner: | changed from Frederico Caldeira Knabben to Piotrek Koszuliński |
---|
comment:10 Changed 11 years ago by
Status: | review → review_passed |
---|
comment:11 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged t/10623b into master (git:cff2ba14dde4).
Revelations (so far):