Bug 120273

Created attachment 209592 [details] Patch

Comment on attachment 209592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209592&action=review > LayoutTests/ChangeLog:8 > + * fast/dom/HTMLDocument/active-element-gets-unforcusable.html: Added. typo in the filenames? "unfoRcusable"

Created attachment 209593 [details] Patch

(In reply to comment #2) > (From update of attachment 209592 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209592&action=review > > > LayoutTests/ChangeLog:8 > > + * fast/dom/HTMLDocument/active-element-gets-unforcusable.html: Added. > > typo in the filenames? "unfoRcusable" Oops!. Fixed in the new patch :)

Seems use case in Bug#120283 can be resolved with this.

Comment on attachment 209593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209593&action=review > Source/WebCore/dom/Document.cpp:381 > +// This class should be passed only to Document::postTask. I don't understand the value of this comment. What other bad thing could someone do with this class. Next time just leave this out. I suspect this entire class will go away, but we if keep it I have a few comments. > Source/WebCore/dom/Document.cpp:386 > + return adoptPtr(new CheckFocusedNodeTask()); I suggest omitting the "()" here. > Source/WebCore/dom/Document.cpp:388 > + virtual ~CheckFocusedNodeTask() { } I suggest omitting this entirely. It should be generated. > Source/WebCore/dom/Document.cpp:391 > + CheckFocusedNodeTask() { } Doesn't seem to be valuable to make this private. There would be no harm if someone constructed one of these without using the create function. I suggest just omitting this and letting it be generated. > Source/WebCore/dom/Document.cpp:395 > + Document* document = toDocument(context); I suggest using Document& here instead of Document* since it can’t be null. In newer code in general we are trying to use references for such things. > Source/WebCore/dom/Document.cpp:401 > + if (focusedElement->renderer() && focusedElement->renderer()->needsLayout()) > + return; This needsLayout check does not make sense. It's not helpful to check if the single renderer has the needs-layout bit set on it. What bug did it fix to check this? > Source/WebCore/dom/Document.cpp:1885 > + // FIXME: Using a Task doesn't look a good idea. Yes, a Task is not a good fix for this. I suggest sharing the updateFocusAppearance timer. > Source/WebCore/dom/Document.cpp:1889 > + if (m_focusedElement && !m_didPostCheckFocusedNodeTask) { > + postTask(CheckFocusedNodeTask::create()); > + m_didPostCheckFocusedNodeTask = true; > + } Here’s what I think this code should say: if (!m_focusedElement->isFocusable()) updateFocusAppearanceSoon(false); Then, inside the updateFocusAppearanceTimerFired function: if (element->isFocusable()) element->updateFocusAppearance(m_updateFocusAppearanceRestoresSelection); else document->setFocusedElement(0); Please give that a try and see if there are any problems caused by that approach.

(In reply to comment #6) > (From update of attachment 209593 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209593&action=review > > > Source/WebCore/dom/Document.cpp:381 > > +// This class should be passed only to Document::postTask. > > I don't understand the value of this comment. What other bad thing could someone do with this class. Next time just leave this out. I suspect this entire class will go away, but we if keep it I have a few comments. > > > Source/WebCore/dom/Document.cpp:386 > > + return adoptPtr(new CheckFocusedNodeTask()); > > I suggest omitting the "()" here. > > > Source/WebCore/dom/Document.cpp:388 > > + virtual ~CheckFocusedNodeTask() { } > > I suggest omitting this entirely. It should be generated. > > > Source/WebCore/dom/Document.cpp:391 > > + CheckFocusedNodeTask() { } > > Doesn't seem to be valuable to make this private. There would be no harm if someone constructed one of these without using the create function. I suggest just omitting this and letting it be generated. > > > Source/WebCore/dom/Document.cpp:395 > > + Document* document = toDocument(context); > > I suggest using Document& here instead of Document* since it can’t be null. In newer code in general we are trying to use references for such things. > > > Source/WebCore/dom/Document.cpp:401 > > + if (focusedElement->renderer() && focusedElement->renderer()->needsLayout()) > > + return; > > This needsLayout check does not make sense. It's not helpful to check if the single renderer has the needs-layout bit set on it. What bug did it fix to check this? > > > Source/WebCore/dom/Document.cpp:1885 > > + // FIXME: Using a Task doesn't look a good idea. > > Yes, a Task is not a good fix for this. I suggest sharing the updateFocusAppearance timer. > > > Source/WebCore/dom/Document.cpp:1889 > > + if (m_focusedElement && !m_didPostCheckFocusedNodeTask) { > > + postTask(CheckFocusedNodeTask::create()); > > + m_didPostCheckFocusedNodeTask = true; > > + } > > Here’s what I think this code should say: > > if (!m_focusedElement->isFocusable()) > updateFocusAppearanceSoon(false); > > Then, inside the updateFocusAppearanceTimerFired function: > > if (element->isFocusable()) > element->updateFocusAppearance(m_updateFocusAppearanceRestoresSelection); > else > document->setFocusedElement(0); > > Please give that a try and see if there are any problems caused by that approach. Darin, Thanks for your comments :) Sure, I will try with m_updateFocusAppearanceTimer and let u know the result.

Comment on attachment 209593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209593&action=review >> Source/WebCore/dom/Document.cpp:1885 >> + // FIXME: Using a Task doesn't look a good idea. > > Yes, a Task is not a good fix for this. I suggest sharing the updateFocusAppearance timer. 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. I'm going to try with a separate timer for this instead of reusing updateFocusAppearance timer. Hope it will be good to do. What do you think?

(In reply to comment #8) > (From update of attachment 209593 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209593&action=review > > >> Source/WebCore/dom/Document.cpp:1885 > >> + // FIXME: Using a Task doesn't look a good idea. > > > > Yes, a Task is not a good fix for this. I suggest sharing the updateFocusAppearance timer. > > 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. > > I'm going to try with a separate timer for this instead of reusing updateFocusAppearance timer. Hope it will be good to do. What do you think? May be with little tweaking it is achievable :) Will try with a recursive guard member.

This loooks like a duplicate of bug 29241.

(In reply to comment #10) > This loooks like a duplicate of bug 29241. Yes, you are right :)

Can we avoid the recursion by ordering the operations appropriately without a recursion guard? Please keep in mind the timer again is *not* recursion. Scheduling timer over and over again infinitely would be a problem, but still, not infinite recursion.

Please keep in mind that scheduling the timer again is *not* recursion (I meant to say). Also lets mark this a duplicate of the older bug. *** This bug has been marked as a duplicate of bug 29241 ***

Created attachment 210012 [details] It is not working in Chrome even it has fix

(In reply to comment #12) > Can we avoid the recursion by ordering the operations appropriately without a recursion guard? Please keep in mind the timer again is *not* recursion. Scheduling timer over and over again infinitely would be a problem, but still, not infinite recursion. (In reply to comment #13) > Please keep in mind that scheduling the timer again is *not* recursion (I meant to say). > Yeah, understood :) > Also lets mark this a duplicate of the older bug. Patch should be submitted to #29241 ? > > *** This bug has been marked as a duplicate of bug 29241 *** Refer the attachment, https://bugs.webkit.org/attachment.cgi?id=210012, it is not working in chrome also. I think Chrome haven't implemented properly it seems.

Created attachment 210015 [details] test case - chrome also not working for certain cases(like calling display:none from timer)

Yes, lets move discussion and further patches to bug 29241.