Issue metadata
Sign in to add a comment
|
HTTPS Error interstitial page blocks Select All context menu
Reported by
larrylac...@yahoo.com,
Mar 19 2018
|
||||||||||||||||||||||
Issue descriptionExample URL: https://cacert.org or badssl.com > Untrusted root Steps to reproduce the problem: 1. Go to https://cacert.org 2. Get a 'Connection not private' warning with NET::ERR_CERT_AUTHORITY_INVALID 3. Tap the NET::ERR tag to display the diagnostic text 4. Long press anywhere in the diagnostic text What is the expected behavior? Get a context menu toolbar at the top with Copy, Select All.. What went wrong? Long press not recognized Did this work before? N/A Chrome version: 65.0.3325.109 Channel: stable OS Version: 5.1.1 Flash Version: Tested on a Samsung tablet SM-T337A with Chrome 63.0.3239.11 and 66.0.3359.0 and reported by other android phone users on 65.0.3325.109. I believe this happens for all sites with a cert connection error. The 2 sites named always fail. On my tablet, on the Chrome tab, the busy-circle icon never stops cycling. I believe it only goes in one direction. On a Windows desktop, the busy-cycle ends after ~3 seconds, and the text is selectable. As an Android works-OK reference point: for chrome://version, a long press opens the copy context menu bar at the top. Without copy/paste of the connection error diagnostic text, it's difficult to troubleshoot user problems.
,
Mar 19 2018
,
Mar 19 2018
Tested the issue in Android and able to reproduce the issue. Steps Followed: 1. Go to https://cacert.org 2. Get a 'Connection not private' warning with NET::ERR_CERT_AUTHORITY_INVALID 3. Tap the NET::ERR tag to display the diagnostic text 4. Long press anywhere in the diagnostic tex 5. Observe no context menu is seen. Chrome versions tested: 65.0.3325.109 (Stable), 67.0.3372.0(Canary) OS: Android 8.1.0, 7.0.0 Android Devices: Pixel XL, Samsung J7 Using the per-revision bisect providing the bisect results, Good Build - 61.0.3117.0 Bad Build - 61.0.3118.0 You are looking for a change made after 476354(GOOD), but before 476355(BAD). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e486658183456c From the CL above, assigning the issue to the owner concerned. @amaralp: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to owner concerned. Please navigate to below link for log's and video-- go/chrome-androidlogs/823173 Note: 1. This issue is not observed in Desktop.
,
Mar 21 2018
Note: If you test this on an 8" tablet, you can also visually observe that the active cycle icon on the tab, never finishes.
,
Mar 21 2018
It looks like |InterstitialPageImpl| doesn't override |RenderViewHostDelegateView::ShowContextMenu()| so the menu selection menu isn't shown. Normally the selection menu is show through |WebContentsViewAndroid::ShowContextMenu()| which overrides that method in |RenderViewHostDelegateView|. I also noticed that dragging the selection handles does not work. Does anyone who works on interstitial know why we don't show context menus?
,
Mar 21 2018
My guess is that context menus were disabled (or, never enabled) since with interstitials being non-committed navigations, the available actions on desktop (view source, print, etc.), would have not matched the interstitial, but the previous page. Interstitials are currently being refactored into committed navigations, and with those the context menu works (since we no longer use InterstitialPageImpl) and actions would match the interstitial, but for the time being we could special case ShowContextMenu for Android, since the problematic elements don't show up there.
,
Mar 21 2018
,
Mar 22 2018
Also note, on large screen android, e.g. Samsung Tablet T8, android 5.1.1, that display tabs: with the interstitial, the busy-cycle icon never completes (displayed in the tap). This 'busy' state may block dragging the handles. Re: non-committed navigation, unless this gate only applies to applies to android, the logic for not displaying the context menu may be inconsistent with the Windows implementation which has a context menu (and completes the busy cycle).
,
Mar 23 2018
I'll take a look at the spinner not completing for interstitials, regarding the context menu, it is disabled on Windows (and other platforms too). (you can try by going to expired.badssl.com and right clicking)
,
Mar 23 2018
Thanks for the investigation. It would good to have a fix for M67, tagging accordingly for tracking purpose. Feel free to modify if needed.
,
Mar 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a17de6fee41d662166d9a807ad33750491542e29 commit a17de6fee41d662166d9a807ad33750491542e29 Author: Pedro Amaral <amaralp@chromium.org> Date: Tue Mar 27 19:43:39 2018 Interstitial call ShowContextMenu on Android Override |ShowContextMenu()| in InterstitialPageImpl and have it call |WebContentsViewAndroid.ShowContextMenu()| on Android. This is done to show the floating text selection menu (see linked bug). Bug: 823173 Change-Id: I01090aee43536cd06c8f8bc28ebccf5c2767d0d8 Reviewed-on: https://chromium-review.googlesource.com/977240 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Carlos IL <carlosil@chromium.org> Commit-Queue: Pedro Amaral <amaralp@chromium.org> Cr-Commit-Position: refs/heads/master@{#546210} [modify] https://crrev.com/a17de6fee41d662166d9a807ad33750491542e29/content/browser/frame_host/interstitial_page_impl.cc [modify] https://crrev.com/a17de6fee41d662166d9a807ad33750491542e29/content/browser/frame_host/interstitial_page_impl.h
,
Mar 27 2018
,
Apr 3 2018
Issue doesn't repro on latest M67, Now we are able to see test selection tool bar. But we do see some other issues like Copy, select all and device hardware back key is not Functional. we have separate bug "issue:828426" to track.
,
Apr 3 2018
Turns out that my fix didn't actually help since the Copy and Select All options don't work (WebContentImpl::GetFocusedFrame() is returning nullptr). This makes me think that copying or selecting all on interstitial pages on Android never worked. I think the correct solution would be to enable context menus on interstitials on all platforms and that would fix Android. I'll revert my patch since showing the menu with broken options is worse than not showing it at all.
,
Apr 3 2018
IMO, it's probably not worth it to tackle this one by itself in that case, as it will be fixed with committed interstitials (which are currently targeted for 68). I will assign this to myself, but feel free to take it if this is needed before 68.
,
Apr 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f10cba2eafb0f3c661f2e3904eb420b01bafa60c commit f10cba2eafb0f3c661f2e3904eb420b01bafa60c Author: Pedro Amaral <amaralp@chromium.org> Date: Wed Apr 04 02:10:20 2018 Revert "Interstitial call ShowContextMenu on Android" This reverts commit a17de6fee41d662166d9a807ad33750491542e29. Reason for revert: The options in the selection menu don't work. Copy and Select All don't do anything. Original change's description: > Interstitial call ShowContextMenu on Android > > Override |ShowContextMenu()| in InterstitialPageImpl and have it call > |WebContentsViewAndroid.ShowContextMenu()| on Android. This is done to > show the floating text selection menu (see linked bug). > > Bug: 823173 > Change-Id: I01090aee43536cd06c8f8bc28ebccf5c2767d0d8 > Reviewed-on: https://chromium-review.googlesource.com/977240 > Reviewed-by: Bo <boliu@chromium.org> > Reviewed-by: Carlos IL <carlosil@chromium.org> > Commit-Queue: Pedro Amaral <amaralp@chromium.org> > Cr-Commit-Position: refs/heads/master@{#546210} TBR=boliu@chromium.org,amaralp@chromium.org,carlosil@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 823173 Change-Id: Ia0743769b372b93acda2835c2274456f3a5379a0 Reviewed-on: https://chromium-review.googlesource.com/994191 Reviewed-by: Pedro Amaral <amaralp@chromium.org> Reviewed-by: Carlos IL <carlosil@chromium.org> Commit-Queue: Pedro Amaral <amaralp@chromium.org> Cr-Commit-Position: refs/heads/master@{#547953} [modify] https://crrev.com/f10cba2eafb0f3c661f2e3904eb420b01bafa60c/content/browser/frame_host/interstitial_page_impl.cc [modify] https://crrev.com/f10cba2eafb0f3c661f2e3904eb420b01bafa60c/content/browser/frame_host/interstitial_page_impl.h
,
Apr 9 2018
,
Aug 1
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pnangunoori@chromium.org
, Mar 19 2018