third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html - Issue 2153063003: Should not call FocusController::setFocusedFrame() for detached frame

Side by Side Diff: third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html

Issue 2153063003: Should not call FocusController::setFocusedFrame() for detached frame (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Proposed fix #3 Created 4 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE HTML>

	2 <script src="../../../resources/testharness.js"></script>

	3 <script src="../../../resources/testharnessreport.js"></script>

	4 <button id="button"></button>

	5 <iframe id="iframe"></iframe>

	6 <script>

	7 // This is a reproduction case for crbug.com/626750.

	8 async_test((test) => {

	9 button.addEventListener('focus', () => {

	10 iframe.contentWindow.focus();

	11 }, false);
	yosin_UTC9 2016/07/21 06:10:10 nit: Since default value of second parameter is \|f nit: Since default value of second parameter is \|false\|, we don't need to explicitly pass it. kochi 2016/07/21 06:55:26 Done. Show quoted text On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: Since default value of second parameter is \|false\|, we don't need to > explicitly pass it. Done.
	12

	13 button.addEventListener('blur', test.step_func_done(() => {

	14 // This detaches <iframe>.

	15 document.open();

	16 assert_true(true, "Test did not crash.");
	yosin_UTC9 2016/07/21 06:10:10 We should check value of \|document.activeElement\| We should check value of \|document.activeElement\| rather than \|assert_true(ture, ...)\| kochi 2016/07/21 06:55:26 What is the intention of wanting to know the then- Show quoted text On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > We should check value of \|document.activeElement\| rather than \|assert_true(ture, > ...)\| What is the intention of wanting to know the then-active focused element? Immediately after document.open(), document.activeElement will always be null? yosin_UTC9 2016/07/21 07:22:36 Checking \|document.activeElement\| leads us \|FocusC Show quoted text On 2016/07/21 at 06:55:26, kochi wrote: > On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > > We should check value of \|document.activeElement\| rather than \|assert_true(ture, > > ...)\| > > What is the intention of wanting to know the then-active focused element? > Immediately after document.open(), document.activeElement will always be null? Checking \|document.activeElement\| leads us \|FocusController\| does right thing after \|document.open()\|. No crash is too weak to check regression. We want to known \|m_focusedFrame\| when setting focus frame is aborted. kochi 2016/07/21 09:00:33 Originally I thought that this test is for reprodu Show quoted text On 2016/07/21 07:22:36, Yosi_UTC9 wrote: > On 2016/07/21 at 06:55:26, kochi wrote: > > On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > > > We should check value of \|document.activeElement\| rather than > \|assert_true(ture, > > > ...)\| > > > > What is the intention of wanting to know the then-active focused element? > > Immediately after document.open(), document.activeElement will always be null? > > Checking \|document.activeElement\| leads us \|FocusController\| does right thing > after > \|document.open()\|. > > No crash is too weak to check regression. We want to known \|m_focusedFrame\| when > setting focus frame is aborted. Originally I thought that this test is for reproducing the crash, and not for testing whether document.open() clears .activeElement or not, and it should be in another layout test, if necessary. Anyway, I found that onfocus handler finishes later than onblur handler, so I added comment about the order of occurrence and changed to use step_func_done for onfocus handler. Added activeElement check next to \|iframe.contentWindow.focus();\| line, which should make sense.
	17 }), false);
	yosin_UTC9 2016/07/21 06:10:10 nit: Since default value of second parameter is \|f nit: Since default value of second parameter is \|false\|, we don't need to explicitly pass it. kochi 2016/07/21 06:55:26 Done. Show quoted text On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: Since default value of second parameter is \|false\|, we don't need to > explicitly pass it. Done.
	18

	19 window.addEventListener('load', () => { button.focus(); }, false);
	yosin_UTC9 2016/07/21 06:10:10 nit: We can write \|window.addEventListener('load', nit: We can write \|window.addEventListener('load', () => button.focus());\| nit: Since default value of second parameter is \|false\|, we don't need to explicitly pass it. kochi 2016/07/21 06:55:26 Done. Show quoted text On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: We can write \|window.addEventListener('load', () => button.focus());\| > nit: Since default value of second parameter is \|false\|, we don't need to > explicitly pass it. Done.
	20 }, "focusing on detached frame should not crash");
	yosin_UTC9 2016/07/21 06:10:10 nit: use single quote since other parts in script nit: use single quote since other parts in script use single quote. kochi 2016/07/21 06:55:26 Done. Show quoted text On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: use single quote since other parts in script use single quote. Done.
	21 </script>

OLD	NEW

« no previous file with comments | « no previous file | third_party/WebKit/Source/core/page/FocusController.cpp » ('j') | third_party/WebKit/Source/core/page/FocusController.cpp » ('J')