Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896463004:
Print Preview: Mac: Do not blur focused item while opening the hamburger menu. (Closed)
DescriptionPrint Preview: Mac: Do not blur focused item while opening the hamburger menu.
:focus selector doesn't work if the container page is inactive. We should
check document.activeElement instead.
BUG=723579
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2896463004
Cr-Commit-Position: refs/heads/master@{#474475}
Committed: https://chromium.googlesource.com/chromium/src/+/a0646d775e805babb8d6949f955071011df709f2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update for activeElement, add a comment #
Total comments: 4
Patch Set 3 : Actually remove a hack for appendChild() #Messages
Total messages: 28 (19 generated)
Description was changed from ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. Do not call listEl.appendChild(itemEl) if the last child is already itemEl. BUG=723579 ========== to ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. Do not call listEl.appendChild(itemEl) if the last child is already itemEl. BUG=723579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tkent@chromium.org changed reviewers: + gene@chromium.org
gene@, would you review this please? My appendChild() behavior change [1] caused crbug.com/723579. Probably the focused element was blurred by removeChild() inside appendChild(). This CL fixes the bug, however I wonder why this CL is necessary because updateListItem_() has the code to keep the focused element. [1] crrev.com/448928
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tkent@chromium.org changed reviewers: + dpapad@chromium.org, vitalybuka@chromium.org
Add more print_preview owners. Would you review this please?
https://codereview.chromium.org/2896463004/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2896463004/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/search/destination_list.js:339: if (listEl.lastChild != itemEl) Without a regression test, and without any comment explaining why this check is necessary, this check is not obvious and it is likely that someone might try to remove it in the future, and silently regress what this CL is fixing. Can we at least add a comment?
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2896463004/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2896463004/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/search/destination_list.js:339: if (listEl.lastChild != itemEl) On 2017/05/23 at 01:02:06, dpapad wrote: > Without a regression test, and without any comment explaining why this check is necessary, this check is not obvious and it is likely that someone might try to remove it in the future, and silently regress what this CL is fixing. > > Can we at least add a comment? I found why :focus didn't work, and Patch Set 2 removed this hacky |if|.
Description was changed from ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. Do not call listEl.appendChild(itemEl) if the last child is already itemEl. BUG=723579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. :focus selector doesn't work if the container page is inactive. We should check document.activeElement instead. BUG=723579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:310: // work in an inactive page. See crbug.com/723579. Out of curiosity, does crbug.com/723579 happen for the new :focus-within pseudo selector too? https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:343: if (listEl.lastChild != itemEl) Did you mean to remove this in patch#2?
https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:310: // work in an inactive page. See crbug.com/723579. On 2017/05/23 at 18:57:30, dpapad wrote: > Out of curiosity, does crbug.com/723579 happen for the new :focus-within pseudo selector too? :focus-within means just "contains a :focus element." So, :focus-within won't fix crbug.com/723579. https://codereview.chromium.org/2896463004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:343: if (listEl.lastChild != itemEl) On 2017/05/23 at 18:57:30, dpapad wrote: > Did you mean to remove this in patch#2? Oops. Removed.
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495668283981440,
"parent_rev": "ed8a8cfa1017522995d6be2961e5dc68064e747f", "commit_rev":
"a0646d775e805babb8d6949f955071011df709f2"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. :focus selector doesn't work if the container page is inactive. We should check document.activeElement instead. BUG=723579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Mac: Do not blur focused item while opening the hamburger menu. :focus selector doesn't work if the container page is inactive. We should check document.activeElement instead. BUG=723579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2896463004 Cr-Commit-Position: refs/heads/master@{#474475} Committed: https://chromium.googlesource.com/chromium/src/+/a0646d775e805babb8d6949f9550... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a0646d775e805babb8d6949f9550... |
