Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860583002:
Properly check visibility of check_connection_header (Closed)
|
Created:
3 years, 7 months ago by Nate Fischer Modified:
3 years, 7 months ago Reviewers:
mmenke CC:
cbentzel+watch_chromium.org, chromium-reviews, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionProperly check visibility of check_connection_header
No change in production logic.
Because the mobile page layout hides elements using 'opacity = 0'
instead of 'display: none', it's not sufficient to only check the
innerText to see if text is visible on the page. Elements with opacity =
0 still show up in body.innerText.
Unfortunately, we can't simply check opacity, because opacity changes
via CSS transition. Elements may still have opacity = 0 immediately
after they're un-hidden, since the CSS transition takes time. Instead,
we check the classList of the details element to determine if it's
visible, since a hidden details element implies that its child nodes are
also hidden.
This CL adds a function for determining the visibility of details and
uses it when checking if
IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed, since
this check isn't compatible with mobile layout.
BUG=717326
Review-Url: https://codereview.chromium.org/2860583002
Cr-Commit-Position: refs/heads/master@{#469758}
Committed: https://chromium.googlesource.com/chromium/src/+/f1a3058bf88ee945ab8e5ead543a3b7450710925
Patch Set 1 #
Total comments: 3
Patch Set 2 : Recursively check visibility of parent elements #
Total comments: 8
Patch Set 3 : Address reviewer comments #Messages
Total messages: 32 (18 generated)
The CQ bit was checked by ntfschr@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.
Description was changed from ========== Properly check visibility of check_connection_header Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. We can't simply check opacity, because opacity changes with a CSS transition. Checking opacity at (approximately) time 0 means non-hidden elements may still have opacity = 0. Therefore, we must check the classList of the details to determine if it's actually visible (and therefore its child nodes are visible) This CL adds a helper for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed. BUG=717326 ========== to ========== Properly check visibility of check_connection_header No change in production logic. Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. Unfortunately, we can't simply check opacity, because opacity changes with a CSS transition. Elements may still have opacity = 0 immediately after they're un-hidden, since the CSS transition takes time. Instead, we check the classList of the details element to see if it's visible, and we say that any of its child nodes are hidden if the details element itself is hidden. This CL adds a function for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed. BUG=717326 ==========
ntfschr@chromium.org changed reviewers: + mmenke@chromium.org
Description was changed from ========== Properly check visibility of check_connection_header No change in production logic. Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. Unfortunately, we can't simply check opacity, because opacity changes with a CSS transition. Elements may still have opacity = 0 immediately after they're un-hidden, since the CSS transition takes time. Instead, we check the classList of the details element to see if it's visible, and we say that any of its child nodes are hidden if the details element itself is hidden. This CL adds a function for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed. BUG=717326 ========== to ========== Properly check visibility of check_connection_header No change in production logic. Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. Unfortunately, we can't simply check opacity, because opacity changes via CSS transition. Elements may still have opacity = 0 immediately after they're un-hidden, since the CSS transition takes time. Instead, we check the classList of the details element to determine if it's visible, since a hidden details element implies that its child nodes are also hidden. This CL adds a function for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed, since this check isn't compatible with mobile layout. BUG=717326 ==========
PTAL
On 2017/05/02 at 22:22:06, Nate Fischer wrote: > PTAL This is another blocker for http://crrev/2848733002 (same reason, since that CL causes browsertests to run against the mobile layout).
https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT IsDisplayingDetails(Browser* browser) { Could we instead make IsDiplayingText check if any parent node of the one displaying the text has the hidden tag? I'm concerned about this regressing and silently causing tests to pass, particularly since it's only used twice.
https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT IsDisplayingDetails(Browser* browser) { On 2017/05/03 at 15:10:59, mmenke wrote: > Could we instead make IsDiplayingText check if any parent node of the one displaying the text has the hidden tag? I'm concerned about this regressing and silently causing tests to pass, particularly since it's only used twice. In general, it seems rather difficult to get a node (and therefore parent nodes) by innerText. For Safe Browsing, we look up nodes by ID: https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... Unfortunately, this particular chunk of text doesn't have an associated ID. So we'd first need to change the tests to check elements with IDs, in addition to adding code to recursively check parent nodes. Is that the route we want to go?
On 2017/05/03 17:54:43, Nate Fischer wrote: > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > File chrome/browser/net/errorpage_browsertest.cc (right): > > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT > IsDisplayingDetails(Browser* browser) { > On 2017/05/03 at 15:10:59, mmenke wrote: > > Could we instead make IsDiplayingText check if any parent node of the one > displaying the text has the hidden tag? I'm concerned about this regressing and > silently causing tests to pass, particularly since it's only used twice. > > In general, it seems rather difficult to get a node (and therefore parent nodes) > by innerText. For Safe Browsing, we look up nodes by ID: > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... > > Unfortunately, this particular chunk of text doesn't have an associated ID. So > we'd first need to change the tests to check elements with IDs, in addition to > adding code to recursively check parent nodes. Is that the route we want to go? Hrm...that sound unpleasant. I'm worried, though, that we may switch to hiding some other parent node instead, and forget to update these tests. Lemme think about it.
On 2017/05/03 at 17:57:38, mmenke wrote: > On 2017/05/03 17:54:43, Nate Fischer wrote: > > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > > File chrome/browser/net/errorpage_browsertest.cc (right): > > > > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > > chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT > > IsDisplayingDetails(Browser* browser) { > > On 2017/05/03 at 15:10:59, mmenke wrote: > > > Could we instead make IsDiplayingText check if any parent node of the one > > displaying the text has the hidden tag? I'm concerned about this regressing and > > silently causing tests to pass, particularly since it's only used twice. > > > > In general, it seems rather difficult to get a node (and therefore parent nodes) > > by innerText. For Safe Browsing, we look up nodes by ID: > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... > > > > Unfortunately, this particular chunk of text doesn't have an associated ID. So > > we'd first need to change the tests to check elements with IDs, in addition to > > adding code to recursively check parent nodes. Is that the route we want to go? > > Hrm...that sound unpleasant. I'm worried, though, that we may switch to hiding some other parent node instead, and forget to update these tests. Lemme think about it. Sure, let me know what you think is best. Thanks, Matt!
Sorry for the delay, made this comment this morning, but forgot to publish it. https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT IsDisplayingDetails(Browser* browser) { On 2017/05/03 17:54:43, Nate Fischer wrote: > On 2017/05/03 at 15:10:59, mmenke wrote: > > Could we instead make IsDiplayingText check if any parent node of the one > displaying the text has the hidden tag? I'm concerned about this regressing and > silently causing tests to pass, particularly since it's only used twice. > > In general, it seems rather difficult to get a node (and therefore parent nodes) > by innerText. For Safe Browsing, we look up nodes by ID: > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... > > Unfortunately, this particular chunk of text doesn't have an associated ID. So > we'd first need to change the tests to check elements with IDs, in addition to > adding code to recursively check parent nodes. Is that the route we want to go? I don't suppose something like this would work: var node = document.evaluate("//div[contains(text(),'ERR_FAILED')]", document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue; domAutomationController.send(node && !node.classList.contains('hidden')) If we're hiding a higher level node, we could also traverse the node hierarchy upwards from the node we find, which will be the innermost one.
The CQ bit was checked by ntfschr@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...
On 2017/05/04 19:03:33, mmenke wrote: > Sorry for the delay, made this comment this morning, but forgot to publish it. > > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > File chrome/browser/net/errorpage_browsertest.cc (right): > > https://codereview.chromium.org/2860583002/diff/1/chrome/browser/net/errorpag... > chrome/browser/net/errorpage_browsertest.cc:110: bool WARN_UNUSED_RESULT > IsDisplayingDetails(Browser* browser) { > On 2017/05/03 17:54:43, Nate Fischer wrote: > > On 2017/05/03 at 15:10:59, mmenke wrote: > > > Could we instead make IsDiplayingText check if any parent node of the one > > displaying the text has the hidden tag? I'm concerned about this regressing > and > > silently causing tests to pass, particularly since it's only used twice. > > > > In general, it seems rather difficult to get a node (and therefore parent > nodes) > > by innerText. For Safe Browsing, we look up nodes by ID: > > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... > > > > Unfortunately, this particular chunk of text doesn't have an associated ID. So > > we'd first need to change the tests to check elements with IDs, in addition to > > adding code to recursively check parent nodes. Is that the route we want to > go? > > I don't suppose something like this would work: > > var node = document.evaluate("//div[contains(text(),'ERR_FAILED')]", document, > null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue; > domAutomationController.send(node && !node.classList.contains('hidden')) > > If we're hiding a higher level node, we could also traverse the node hierarchy > upwards from the node we find, which will be the innermost one. PTAL Yeah, that pointed me in the correct direction. I made a slight modification to your xpath query to get things working for all the test cases. Interestingly, the xpath query is safe to keep case sensitive (despite crbug/xxx). While we needed case insensitive checks when we were using document.body.innerText, we don't need it for xpath queries. We used CSS text-transforms to capitalize buttons for mobile layout, which apparently affects innerText of parent elements. This CSS text-transform does not affect xpath queries, however (great news).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! I'll plan to take another look before you land, but I don't feel like this CL really needs to be blocked on me taking another look. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:100: if (!node) return 'node not found'; nit: split condition onto two lines. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:100: if (!node) return 'node not found'; This seems a little weird to me. Can we instead do: if (!node) return false; ... if (!node.parentElement) return true; return isNodeVisible(node.parentElement); (Could also combine the final three above lines, if you prefer) https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:106: } nit: Just for consistency with this file, may want to not use braces for this if.
https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:111: var isVisible = Boolean(node && isNodeVisible(node)); If you follow my suggestions above, this just becomes: var isVisible = idNodeVisible(node); And could even optionally inline it below.
The CQ bit was checked by ntfschr@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...
Thanks, Matt! I tested this against mobile and desktop layouts and everything seems good. Let me know if you have any other suggestions, otherwise I'll land after the dry run passes. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... File chrome/browser/net/errorpage_browsertest.cc (right): https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:100: if (!node) return 'node not found'; On 2017/05/05 14:47:20, mmenke wrote: > nit: split condition onto two lines. Done. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:100: if (!node) return 'node not found'; On 2017/05/05 14:47:20, mmenke wrote: > This seems a little weird to me. > > Can we instead do: > > if (!node) > return false; > ... > if (!node.parentElement) > return true; > return isNodeVisible(node.parentElement); > > (Could also combine the final three above lines, if you prefer) Done. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:106: } On 2017/05/05 14:47:20, mmenke wrote: > nit: Just for consistency with this file, may want to not use braces for this > if. Done. https://codereview.chromium.org/2860583002/diff/20001/chrome/browser/net/erro... chrome/browser/net/errorpage_browsertest.cc:111: var isVisible = Boolean(node && isNodeVisible(node)); On 2017/05/05 14:52:44, mmenke wrote: > If you follow my suggestions above, this just becomes: > > var isVisible = idNodeVisible(node); > > And could even optionally inline it below. Done.
Still LGTM, thanks for taking the extra time to do this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ntfschr@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": 1494012693297990,
"parent_rev": "6a56a0452c030b3594f3b5dbe9ec77cd8c74b5db", "commit_rev":
"f1a3058bf88ee945ab8e5ead543a3b7450710925"}
Message was sent while issue was closed.
Description was changed from ========== Properly check visibility of check_connection_header No change in production logic. Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. Unfortunately, we can't simply check opacity, because opacity changes via CSS transition. Elements may still have opacity = 0 immediately after they're un-hidden, since the CSS transition takes time. Instead, we check the classList of the details element to determine if it's visible, since a hidden details element implies that its child nodes are also hidden. This CL adds a function for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed, since this check isn't compatible with mobile layout. BUG=717326 ========== to ========== Properly check visibility of check_connection_header No change in production logic. Because the mobile page layout hides elements using 'opacity = 0' instead of 'display: none', it's not sufficient to only check the innerText to see if text is visible on the page. Elements with opacity = 0 still show up in body.innerText. Unfortunately, we can't simply check opacity, because opacity changes via CSS transition. Elements may still have opacity = 0 immediately after they're un-hidden, since the CSS transition takes time. Instead, we check the classList of the details element to determine if it's visible, since a hidden details element implies that its child nodes are also hidden. This CL adds a function for determining the visibility of details and uses it when checking if IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER is displayed, since this check isn't compatible with mobile layout. BUG=717326 Review-Url: https://codereview.chromium.org/2860583002 Cr-Commit-Position: refs/heads/master@{#469758} Committed: https://chromium.googlesource.com/chromium/src/+/f1a3058bf88ee945ab8e5ead543a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f1a3058bf88ee945ab8e5ead543a... |
