1. Try to focus on HTMLInput element 2. Than hide it for example with js 3. You can still type in it & focus is still there In DOM spec you should blur input. Sincerely yours, YouTrack Team http://www.jetbrains.com/youtrack/index.html
*** Bug 120273 has been marked as a duplicate of this bug. ***
(In reply to comment #8 in bug 120273) > I tried using updateFocusAppearance timer, but Document::updateFocusAppearanceTimerFired calls Document::updateLayout, from Document::updateLayout() again we are scheduling a timer. It cases above 2 functions being called repeatedly. Please explain in more detail why the two functions are called repeatedly. I’d assume that after the first time there would no longer be a focused element, so there would be no repeated call.
(In reply to comment #2) > (In reply to comment #8 in bug 120273) > > I tried using updateFocusAppearance timer, but Document::updateFocusAppearanceTimerFired calls Document::updateLayout, from Document::updateLayout() again we are scheduling a timer. It cases above 2 functions being called repeatedly. > > Please explain in more detail why the two functions are called repeatedly. I’d assume that after the first time there would no longer be a focused element, so there would be no repeated call. void Document::updateFocusAppearanceTimerFired(Timer<Document>*) { Element* element = focusedElement(); if (!element) return; updateLayout(); >> From this timer callback Document::updateLayout is called before resetting the focus. Again from Document::updateLayout() updateFocusAppearance timer will be scheduled. if (element->isFocusable()) element->updateFocusAppearance(m_updateFocusAppearanceRestoresSelection); else setFocusedElement(0); >> I'm don't think calling a updateLayout() from this statement is good idea. } Also once the Element is hidden using "display: none", from the rendererObject detach Document::cancelFocusAppearanceUpdate() is been called, which stops the updateFocusAppearance timer.(so we can't reuse updateFocusAppearance timer anyway) What I'm currently trying is scheduling a separate timer from Document::cancelFocusAppearanceUpdate() to reset the focus. It works fine for "display: none", but not for "visibility:hidden". I'm trying to make it work for "visibility: hidden" and "width:0px" || "height: 0px";
Created attachment 210164 [details] Patch
(In reply to comment #4) > Created an attachment (id=210164) [details] > Patch Chrome is not passing the modified test-case.(i.e setting display:none,visibility:hidden via timer).
Comment on attachment 210164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210164&action=review I’m going to say review+, but I don’t think this is 100% right. > Source/WebCore/dom/Document.cpp:1829 > + // Style change may reset the focus, e.g. display: none, visibility: hidden. > + if (!m_resetHiddenFocusElementTimer.isActive()) > + m_resetHiddenFocusElementTimer.startOneShot(0); I’m not sure this is the right place to hook this in. For one thing, this function doesn’t always update style, so it’s strange to unconditionally set this timer every time. For another, layout changes could also have an effect, so I don’t understand why having it only in the updateStyleIfNeeded function is sufficient. This should go in the right place, not just a place that happens to work. We should think about exactly what can affect isFocusable and put this in the one or more bottleneck functions that cover all of those things, but not set up the timer if if there is no focused element or no actual change. > Source/WebCore/dom/Document.cpp:4714 > + Element* element = focusedElement(); > + if (!element) > + return; > + > + if (!element->isFocusable()) > + setFocusedElement(0); It’s a little strange to use early return for the case of no focused element, but use a nested if statement for the “is still focusable” element. It could and should just be all one if statement. Also, within the class I think it’s better to use the data member rather than a public function. if (m_focusedElement && !m_focusedElement->isFocusable()) setFocusedElement(0);
(In reply to comment #6) > (From update of attachment 210164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210164&action=review > > I’m going to say review+, but I don’t think this is 100% right. > > > Source/WebCore/dom/Document.cpp:1829 > > + // Style change may reset the focus, e.g. display: none, visibility: hidden. > > + if (!m_resetHiddenFocusElementTimer.isActive()) > > + m_resetHiddenFocusElementTimer.startOneShot(0); > > I’m not sure this is the right place to hook this in. Initially I felt Document::cancelFocusAppearanceUpdate() was the right place, But unfortunately it wasn't called for "visibility:hidden". I will try to find-out the possibilities to call Document::cancelFocusAppearanceUpdate() from appropriate place to satisfy the "visibility: hidden" case. Once it is done, we can start a timer to reset a focus from Document::cancelFocusAppearanceUpdate(). > > For one thing, this function doesn’t always update style, so it’s strange to unconditionally set this timer every time. > > For another, layout changes could also have an effect, so I don’t understand why having it only in the updateStyleIfNeeded function is sufficient. I'm not sure t how layout could affect the focus state, do you mean setting width:0px || height:0px to an element should unfocus it? > > This should go in the right place, not just a place that happens to work. We should think about exactly what can affect isFocusable and put this in the one or more bottleneck functions that cover all of those things, but not set up the timer if if there is no focused element or no actual change. Definitely. As mentioned earlier, I will try to call Document::cancelFocusAppearanceUpdate() for "visibility:hidden" also. > > > Source/WebCore/dom/Document.cpp:4714 > > + Element* element = focusedElement(); > > + if (!element) > > + return; > > + > > + if (!element->isFocusable()) > > + setFocusedElement(0); > > It’s a little strange to use early return for the case of no focused element, but use a nested if statement for the “is still focusable” element. It could and should just be all one if statement. Also, within the class I think it’s better to use the data member rather than a public function. > > if (m_focusedElement && !m_focusedElement->isFocusable()) > setFocusedElement(0); Sure, I will change this :)
(In reply to comment #7) > I'm not sure t how layout could affect the focus state, do you mean setting width:0px || height:0px to an element should unfocus it? Read HTMLFormControlElement::isFocusable, for example. It checks the renderer’s size. I am not an expressing an opinion about whether it should do that or not. But it does. And if we haven’t done layout, we can’t check a renderer’s size.
(In reply to comment #7) > Initially I felt Document::cancelFocusAppearanceUpdate() was the right place I don’t think that is the right place, and I am not sure why you do. > > This should go in the right place, not just a place that happens to work. We should think about exactly what can affect isFocusable and put this in the one or more bottleneck functions that cover all of those things, but not set up the timer if if there is no focused element or no actual change. > Definitely. As mentioned earlier, I will try to call Document::cancelFocusAppearanceUpdate() for "visibility:hidden" also. This new logic needs to run any time we possibly changed what isFocusable will return for the focused element. I’m don’t think that cancelFocusAppearanceUpdate needs to be called in those same places, though. We don’t just want to cancel that timer because something might have changed. I don’t think you’re on the right track here.
Created attachment 210193 [details] Patch
(In reply to comment #9) > (In reply to comment #7) > > Initially I felt Document::cancelFocusAppearanceUpdate() was the right place > > I don’t think that is the right place, and I am not sure why you do. > > > > This should go in the right place, not just a place that happens to work. We should think about exactly what can affect isFocusable and put this in the one or more bottleneck functions that cover all of those things, but not set up the timer if if there is no focused element or no actual change. > > Definitely. As mentioned earlier, I will try to call Document::cancelFocusAppearanceUpdate() for "visibility:hidden" also. > > This new logic needs to run any time we possibly changed what isFocusable will return for the focused element. > > I’m don’t think that cancelFocusAppearanceUpdate needs to be called in those same places, though. We don’t just want to cancel that timer because something might have changed. I don’t think you’re on the right track here. Without seeing your comment I uploaded a patch. I will upload a new patch which meets those scenarios.
Created attachment 210200 [details] Patch
(In reply to comment #12) > Created an attachment (id=210200) [details] > Patch The patch handles the both layout & style change.
Created attachment 212195 [details] Patch
Comment on attachment 212195 [details] Patch One risk is that some call sites might call layout directly on the FrameView rather than through Document.
Comment on attachment 212195 [details] Patch Clearing flags on attachment: 212195 Committed r156185: <http://trac.webkit.org/changeset/156185>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 121727
This caused a lot of crashes on bots: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156186%20(10285)/results.html>. Also, the test from this patch fails: <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r156208%20(11940)/fast/dom/HTMLDocument/active-element-gets-unfocusable-pretty-diff.html>. Rolling out in a moment.
Rolled out in <https://trac.webkit.org/r156210>.
Created attachment 212271 [details] Patch
(In reply to comment #19) > This caused a lot of crashes on bots: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156186%20(10285)/results.html>. Sorry for breaking the LayoutTests. Failures were due the Assertions in method " Element::isFocusable ()" to confirm the Layout is not required in that state, (I didn't test the patch in the Debug build). New patch considers that scenario as well (thanks to https://chromium.googlesource.com/chromium/blink/+/c58f636fd18fc27944c42e27d6a92a36867c57e1). > > Also, the test from this patch fails: <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r156208%20(11940)/fast/dom/HTMLDocument/active-element-gets-unfocusable-pretty-diff.html>. > http://trac.webkit.org/r155250 moved the js-test-pre.js & js-test-post.js to LayoutTests/resources. That's why it was failing. > Rolling out in a moment.
Comment on attachment 212271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review > Source/WebCore/dom/Document.cpp:4719 > + if (element->renderer() && element->renderer()->needsLayout()) > + return; This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree. Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong.
Comment on attachment 212271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review >> Source/WebCore/dom/Document.cpp:4719 >> + return; > > This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree. > > Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong. it’s *not* a fully correct fix
Comment on attachment 212271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review >>> Source/WebCore/dom/Document.cpp:4719 >>> + return; >> >> This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree. >> >> Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong. > > it’s *not* a fully correct fix A correct fix would be something more like this: if (FrameView* view = this->view()) { if (view->needsLayout()) return; } And also, we have to add code to make sure that this “reset hidden focus element logic” runs when the layout is done.
*** Bug 112992 has been marked as a duplicate of this bug. ***
(In reply to comment #25) > (From update of attachment 212271 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review > > >>> Source/WebCore/dom/Document.cpp:4719 > >>> + return; > >> > >> This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree. > >> > >> Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong. > > > > it’s *not* a fully correct fix > > A correct fix would be something more like this: > > if (FrameView* view = this->view()) { > if (view->needsLayout()) > return; > } > > And also, we have to add code to make sure that this “reset hidden focus element logic” runs when the layout is done. Yes, already resetHiddenFocusElementSoon() is called(from Document::updateLayout) after finishing the layout. // Only do a layout if changes have occurred that make it necessary. if (frameView && renderView() && (frameView->layoutPending() || renderView()->needsLayout())) **frameView->layout();** // Active focus element's isFocusable() state may change after Layout. e.g. width: 0px or height: 0px. **resetHiddenFocusElementSoon();**
Created attachment 212303 [details] Patch
Comment on attachment 212303 [details] Patch Clearing flags on attachment: 212303 Committed r156252: <http://trac.webkit.org/changeset/156252>
Looks like this patch broke facebook.com on iOS. When the user taps on the status on facebook.com, the page focuses an textarea, which is then made momentarily visible by setting display:none. The page then removes display:none later and expects the textarea to be still focused. With this patch, the focus is removed after the page sets display:none and the keyboard disappears. Since the focus is never reset on the textarea, the user can never type in anything. The specification may need to change here given that this (rather odd) behavior/expectation exists on one of the most popular websites on the Web.
*** Bug 216248 has been marked as a duplicate of this bug. ***
Any change here should wait on the resolution of https://github.com/w3c/uievents/issues/236
*** Bug 40338 has been marked as a duplicate of this bug. ***
Bug 216418 is the same issue, but with "visibility: hidden".