Issue metadata
Sign in to add a comment
|
[desktop-pwas] Page action popovers should be aligned to the page action button |
||||||||||||||||||||||||
Issue descriptionChrome Version: 66.0.3344.0 OS: Chrome - Page action popovers need to be slightly above the title bar. - Right-aligned to the corresponding page action button. - The page action button needs to be highlighted when they are open. As shown in mock. See also: Issue 814531 (for 3-dot-menu-anchored popovers).
,
Feb 27 2018
This needs guidance from bettes@: Background: - In the regular Chrome with Harmony, the popoover dialogs that come from the page action icons are aligned to the right edge of *Omnibox* (not to the page action icon). - Desktop PWAs don't have omnibox Question: - Where to align popovers from page actions in Desktop PWAs? Option 1: Always align to the right edge of the 3-dot menu Option 2: Align to the right edge of the associated page action
,
Mar 2 2018
from bettes@: align to the anchor (not 3-dot menu) since there's no border (in chrome there's omnibox border - so that's a difference)
,
Mar 2 2018
,
Mar 26 2018
,
Mar 27 2018
,
Mar 27 2018
Merged in Issue 809280 . This issue now covers both anchoring popovers to the page action button AND if there is no page action button (e.g., just a permission being requested) that it be aligned to the 3-dot menu. Re-attaching spec from the other issue.
,
Apr 6 2018
Pushing back to P3 (this is less urgent than the other P2s).
,
Apr 11 2018
Screenshots for a WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/1004886
,
Apr 11 2018
re: c10, it might be helpful to check out the fix with chrome://flags/#secondary-ui-md ENABLED. The flag will *remove the caret* above the X, which is *intended*, and it should still highlight the 3-dot menu like c#8. Thanks alancutter@!
,
Apr 11 2018
Is secondary-ui-md enabled by default for 67? (If not then this won't matter, but maybe you're just asking to make sure it looks good both ways?)
,
Apr 11 2018
,
Apr 11 2018
c12 - >> Is secondary-ui-md enabled by default for 67? From my recent testing last week and this week, it was not default enabled on CrOS. Adding abdulsyed@ for the correct answer. abdulsyed@, is it enabled by defualt for 67? Also, do you mind letting us know if secondary-ui-md is targetting M67? Thanks! >> maybe you're just asking to make sure it looks good both ways? That's correct since secondary-ui-md launch seems getting closer.
,
Apr 12 2018
OK but let's not worry about all the flags that have not been turned on.
,
Apr 13 2018
67 has branched, moving bugs over to 68.
,
Apr 13 2018
This might make it; moving back to 67.
,
Apr 13 2018
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f591313ca81261589199eb545ca74eee09ef4d12 commit f591313ca81261589199eb545ca74eee09ef4d12 Author: Alan Cutter <alancutter@chromium.org> Date: Fri Apr 13 04:22:29 2018 Fix permission bubble anchoring for hosted app windows This CL fixes the anchor point for permission bubbles in hosted app windows where there is no location bar to anchor to. Instead they anchor to the app menu button in the title bar. Previously they would anchor to the top left of the window. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=333976&signed_aid=xJPvJxrv89kmG6kAZhF0BQ==&inline=1 After (without location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=333977&signed_aid=9OR5wJQecvy4ZcBjUBU7hA==&inline=1 After (with location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=334359&signed_aid=dcPRBhCpeAmfmG5ixtDJ_w==&inline=1 This change does not affect regular tabbed browser windows. Bug: 814532 Change-Id: I51f87b710764d4f156834e4fa9ce0e7e10450ad8 Reviewed-on: https://chromium-review.googlesource.com/1004886 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/heads/master@{#550514} [modify] https://crrev.com/f591313ca81261589199eb545ca74eee09ef4d12/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
Apr 13 2018
We'll wait and see if this makes it into branch. Then consider merging.
,
Apr 13 2018
I don't think this is merge worthy, it doesn't impact accessibility or security.
,
Apr 16 2018
This didn't make it into branch. CL is +6,-8 so should be mergeable. Will wait to verify on Canary.
,
Apr 16 2018
On reconsideration this might be merge worthy from a security standpoint as it makes the bubble overlap with the browser chrome more obvious.
,
Apr 17 2018
OK. Canary isn't updated yet. Will wait.
,
Apr 17 2018
This broke permission prompts in fullscreen: issue 833257
,
Apr 17 2018
Hmm, OK thanks for looking at that. Seems we'll have to either merge this AND the fix, or neither. Let's decide once the fix for that lands.
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f591313ca81261589199eb545ca74eee09ef4d12 commit f591313ca81261589199eb545ca74eee09ef4d12 Author: Alan Cutter <alancutter@chromium.org> Date: Fri Apr 13 04:22:29 2018 Fix permission bubble anchoring for hosted app windows This CL fixes the anchor point for permission bubbles in hosted app windows where there is no location bar to anchor to. Instead they anchor to the app menu button in the title bar. Previously they would anchor to the top left of the window. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=333976&signed_aid=xJPvJxrv89kmG6kAZhF0BQ==&inline=1 After (without location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=333977&signed_aid=9OR5wJQecvy4ZcBjUBU7hA==&inline=1 After (with location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=334359&signed_aid=dcPRBhCpeAmfmG5ixtDJ_w==&inline=1 This change does not affect regular tabbed browser windows. Bug: 814532 Change-Id: I51f87b710764d4f156834e4fa9ce0e7e10450ad8 Reviewed-on: https://chromium-review.googlesource.com/1004886 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/heads/master@{#550514} [modify] https://crrev.com/f591313ca81261589199eb545ca74eee09ef4d12/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
Apr 20 2018
Hi Alan, What was this merge? (What is testbranch?)
,
Apr 23 2018
This was not merged, I don't know why bugdroid1 added the merge-merged-testbranch label.
,
Apr 23 2018
,
Apr 23 2018
,
Apr 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6cad4d4bf70544914fe9d586640b13875feb78f7 commit 6cad4d4bf70544914fe9d586640b13875feb78f7 Author: Alan Cutter <alancutter@chromium.org> Date: Mon Apr 23 18:57:03 2018 Move comment in GetPageInfoAnchorView for clarity This CL implements a review change that was missed in: https://chromium-review.googlesource.com/c/chromium/src/+/1004886 This adds whitespace to associate a comment with an if block for clarity, there are no behavioural changes in this CL. Bug: 814532 Change-Id: Ieb234227dd82c070889a28becad104b018c1833d Reviewed-on: https://chromium-review.googlesource.com/1011464 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#552774} [modify] https://crrev.com/6cad4d4bf70544914fe9d586640b13875feb78f7/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
Apr 24 2018
#33: I think it's gotten a bit too complex for merging, I would leave it out.
,
Apr 24 2018
,
Apr 24 2018
#33: If the reply to https://bugs.chromium.org/p/chromium/issues/detail?id=825073#c32 says it's a security blocker then this should be merged, otherwise I wouldn't.
,
Apr 24 2018
Security said it's a (soft?) blocker. I think we should merge it. Which CLs need to be merged in this case? - r550514 ? - https://crrev.com/c/1013676 (not yet landed) ? I hope we don't also have to merge r552774.
,
Apr 24 2018
No, as discussed leading up to #38, this has not been merged yet, due to complications. Security have requested a merge so we're going to try and get it in. Thanks for filing Issue 836429 for secondary-ui-md. That should not affect M67 as the secondary UI flag is not enabled. I will triage.
,
Apr 26 2018
Requesting to merge commit f591313ca81261589199eb545ca74eee09ef4d12. This has been in canary long enough for a regression to be found (see issue 833257 ). The fix for the regression will also request merge.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/780a71ed089233d25709d37f382191e905c55b6f commit 780a71ed089233d25709d37f382191e905c55b6f Author: Alan Cutter <alancutter@chromium.org> Date: Thu Apr 26 03:32:41 2018 Fix permission prompt anchoring in fullscreen when UI is hidden This CL fixes a regression caused by https://chromium-review.googlesource.com/1004886 where permission prompts would appear partially off screen in full screen mode on platforms where we hide the Chrome UI. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334665&signed_aid=cQIe_b4bfUZn8i-akRK_ZQ==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334829&signed_aid=cXhOlJe-_oibvrzmLtL4Bw==&inline=1 This also fixes the DCHECK in GetPageInfoAnchorRect() to be relative to GetPageInfoAnchorView() rather than copy pasting its bail out condition. Bug: 833257 , 814532 Change-Id: I6ef85ace735b0b217b3a473365791ad453cf8378 Reviewed-on: https://chromium-review.googlesource.com/1013676 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#553898} [modify] https://crrev.com/780a71ed089233d25709d37f382191e905c55b6f/chrome/browser/ui/extensions/hosted_app_browsertest.cc [modify] https://crrev.com/780a71ed089233d25709d37f382191e905c55b6f/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
Apr 26 2018
Requesting to merge 780a71ed089233d25709d37f382191e905c55b6f as well which fixes a regression in f591313ca81261589199eb545ca74eee09ef4d12.
,
Apr 26 2018
This has been around for a while and isn't a M67 regression. Have both CLs been adequately tested to ensure no further anticipated issues? Prefer not to merge for these reasons given it's a cosmetic issue. Re-tag with details if otherwise.
,
Apr 27 2018
Security review has requested that this UI fix gets merged: https://bugs.chromium.org/p/chromium/issues/detail?id=825073#c34 Though cosmetic it relates to user permissions, the security related part of this fix is to make the dialog to overlap with outside of the renderer frame so it can't be spoofed as easily.
,
Apr 27 2018
The bug is marked as P3 or Feature. It should not be merged as M67 is in beta. Please contact the approriate milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 27 2018
This is needed for security review, updating priority accordingly.
,
Apr 27 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 27 2018
Did this get launched prior to that review? Why is this critical for M67 rather than a punted feature? Approvals and testing should be in place by now.
,
Apr 30 2018
#49 I will reply on Issue 825073 where we can discuss the launch process.
,
Apr 30 2018
Please update this bug when more information is available. Waiting for update on #49 to approve.
,
May 1 2018
#51: Still waiting for a response from kbleicher. Myself and estark@ have responded on Issue 825073.
,
May 1 2018
cindyb@ is shadowing m67. I reviewed Issue 825073; thanks for those details. Approving merge to M67 Chrome OS.
,
May 2 2018
#53 Thanks very much for looking into it!
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38dbcd56379722f55b1596c1075fa898400849a9 commit 38dbcd56379722f55b1596c1075fa898400849a9 Author: Alan Cutter <alancutter@chromium.org> Date: Wed May 02 06:18:10 2018 Fix permission bubble anchoring for hosted app windows This CL fixes the anchor point for permission bubbles in hosted app windows where there is no location bar to anchor to. Instead they anchor to the app menu button in the title bar. Previously they would anchor to the top left of the window. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=333976&signed_aid=xJPvJxrv89kmG6kAZhF0BQ==&inline=1 After (without location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=333977&signed_aid=9OR5wJQecvy4ZcBjUBU7hA==&inline=1 After (with location bar): https://bugs.chromium.org/p/chromium/issues/attachment?aid=334359&signed_aid=dcPRBhCpeAmfmG5ixtDJ_w==&inline=1 This change does not affect regular tabbed browser windows. Bug: 814532 Change-Id: I51f87b710764d4f156834e4fa9ce0e7e10450ad8 Reviewed-on: https://chromium-review.googlesource.com/1004886 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Alan Cutter <alancutter@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#550514}(cherry picked from commit f591313ca81261589199eb545ca74eee09ef4d12) Reviewed-on: https://chromium-review.googlesource.com/1039115 Reviewed-by: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#436} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/38dbcd56379722f55b1596c1075fa898400849a9/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/054cf57f1e1d89c99445721f27150d0088de82c1 commit 054cf57f1e1d89c99445721f27150d0088de82c1 Author: Alan Cutter <alancutter@chromium.org> Date: Wed May 02 06:39:18 2018 Fix permission prompt anchoring in fullscreen when UI is hidden This CL fixes a regression caused by https://chromium-review.googlesource.com/1004886 where permission prompts would appear partially off screen in full screen mode on platforms where we hide the Chrome UI. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334665&signed_aid=cQIe_b4bfUZn8i-akRK_ZQ==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334829&signed_aid=cXhOlJe-_oibvrzmLtL4Bw==&inline=1 This also fixes the DCHECK in GetPageInfoAnchorRect() to be relative to GetPageInfoAnchorView() rather than copy pasting its bail out condition. TBR=alancutter@chromium.org (cherry picked from commit 780a71ed089233d25709d37f382191e905c55b6f) Bug: 833257 , 814532 Change-Id: I6ef85ace735b0b217b3a473365791ad453cf8378 Reviewed-on: https://chromium-review.googlesource.com/1013676 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#553898} Reviewed-on: https://chromium-review.googlesource.com/1039204 Reviewed-by: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#437} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/054cf57f1e1d89c99445721f27150d0088de82c1/chrome/browser/ui/extensions/hosted_app_browsertest.cc [modify] https://crrev.com/054cf57f1e1d89c99445721f27150d0088de82c1/chrome/browser/ui/views/bubble_anchor_util_views.cc
,
May 14 2018
Removing obsolete merge labels (note: the Reject was from an earlier merge review; the most recent advice from release manager was Merge-Approved). |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by calamity@chromium.org
, Feb 27 2018