Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774012:
Support for clipped experimental popup repositioning (Closed)
|
Created:
10 years, 8 months ago by Jeff Timanus Modified:
9 years, 7 months ago CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, ben+cc_chromium.org Base URL:
svn://chrome-svn.corp.google.com/chrome/trunk/src/ Visibility:
Public. |
DescriptionSimple change that re-positions the arrow location on displayed extension popup views. This change allows popups to re-orient themselves if they are partially clipped by the working monitor. Note that this only applies to popups created via the experimental extension popup API.
BUG=42779
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47760
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 11
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 1
Patch Set 9 : '' #
Total comments: 2
Patch Set 10 : '' #
Total comments: 2
Patch Set 11 : '' #
Messages
Total messages: 15 (0 generated)
+ Reviewers Erik & Aaron, Please review this change, if you have a moment. Jeff
Taking a step back here, this is a really big change to the layout & positioning of popups. I'd like to ask the question: why is it so bad that popups may open with some area off the screen? Presumably, the browser will also be largely off the screen as well. Can you upload a screenshot of the worst case to the bug? On Thu, Apr 29, 2010 at 11:57 AM, <twiz@chromium.org> wrote: > + Reviewers Erik & Aaron, > > Please review this change, if you have a moment. > > Jeff > > > > http://codereview.chromium.org/1774012/show >
I uploaded a new revision that makes use of a delegate interface to reposition only those popups created via the experimental API. I also cleaned up some of the code that was explicitly referencing the ArrowLocation values, and instead used the helper functions in BrowserBubble. Please have a look when you have a moment.
The logic here to change the position of the anchor seems brittle, and also likely to result in strange "jumping" around behavior if the window is slowly dragged so that the relative_to_ view is moved entirely off screen by enough. It seems like you ought to be able to calculate if a change in anchor position will result in an increase in popup view area shown on the monitor and then change it if so. http://codereview.chromium.org/1774012/diff/18001/19001 File chrome/browser/extensions/extension_popup_api.cc (right): http://codereview.chromium.org/1774012/diff/18001/19001#newcode159 chrome/browser/extensions/extension_popup_api.cc:159: if (popup_rect.IsEmpty()) please note on the comment above the declaration of ExtensionPopupResized (in the interface) that it may be called with empty size (and why). http://codereview.chromium.org/1774012/diff/18001/19002 File chrome/browser/views/extensions/extension_popup.cc (right): http://codereview.chromium.org/1774012/diff/18001/19002#newcode51 chrome/browser/views/extensions/extension_popup.cc:51: } // unnamed namespace nit: just "namespace" http://codereview.chromium.org/1774012/diff/18001/19002#newcode185 chrome/browser/views/extensions/extension_popup.cc:185: // space, so convert the screen-space bounds back to view space. This comment is unnecessary. http://codereview.chromium.org/1774012/diff/18001/19002#newcode312 chrome/browser/views/extensions/extension_popup.cc:312: // Convert the relative position to screen coordinates. also unnecessary. http://codereview.chromium.org/1774012/diff/18001/19002#newcode333 chrome/browser/views/extensions/extension_popup.cc:333: int y = relative_rect.bottom(); nit: keep the the if { } else {} structure here. (don't assign, then re-assign on if)
Thanks for your review, Rafael. I made all of the changes you suggested. As for the brittleness of the repositioning logic, my current approach is similar to what you suggest: I first determine if the popup is clipped by the screen, and then toggle the arrow location. If the popup is still clipped in the new orientation, then I revert back to the initial arrow position. Note that toggling the arrow location will not result in a flicker of the popup - setting this value does not post an immediate redraw: It only changes how the bounds calculations are performed. The popup will not 'jump' unless that jump results in a fully unclipped popup. http://codereview.chromium.org/1774012/diff/18001/19001 File chrome/browser/extensions/extension_popup_api.cc (right): http://codereview.chromium.org/1774012/diff/18001/19001#newcode159 chrome/browser/extensions/extension_popup_api.cc:159: if (popup_rect.IsEmpty()) On 2010/05/17 19:11:34, rafaelw wrote: > please note on the comment above the declaration of ExtensionPopupResized (in > the interface) that it may be called with empty size (and why). Done. http://codereview.chromium.org/1774012/diff/18001/19002 File chrome/browser/views/extensions/extension_popup.cc (right): http://codereview.chromium.org/1774012/diff/18001/19002#newcode51 chrome/browser/views/extensions/extension_popup.cc:51: } // unnamed namespace On 2010/05/17 19:11:34, rafaelw wrote: > nit: just "namespace" Done. http://codereview.chromium.org/1774012/diff/18001/19002#newcode185 chrome/browser/views/extensions/extension_popup.cc:185: // space, so convert the screen-space bounds back to view space. On 2010/05/17 19:11:34, rafaelw wrote: > This comment is unnecessary. Done. http://codereview.chromium.org/1774012/diff/18001/19002#newcode312 chrome/browser/views/extensions/extension_popup.cc:312: // Convert the relative position to screen coordinates. On 2010/05/17 19:11:34, rafaelw wrote: > also unnecessary. Done. http://codereview.chromium.org/1774012/diff/18001/19002#newcode333 chrome/browser/views/extensions/extension_popup.cc:333: int y = relative_rect.bottom(); On 2010/05/17 19:11:34, rafaelw wrote: > nit: keep the the if { } else {} structure here. (don't assign, then re-assign > on if) Done.
What I'm suggesting is that you shouldn't need to *try* repositioning the anchor and then check to see if it improved things. You should be able to calculate the optimum position and set it if it isn't the current position. Checking to see if the intersection rect is different in width or height from the view rect is brittle, for instance, in the case that the rect is entirely off the screen, or if the rect is larger than the screen in one dimension (and spanning it). On Mon, May 17, 2010 at 2:00 PM, <twiz@chromium.org> wrote: > Thanks for your review, Rafael. > > I made all of the changes you suggested. > > As for the brittleness of the repositioning logic, my current approach is > similar to what you suggest: I first determine if the popup is clipped by > the > screen, and then toggle the arrow location. If the popup is still clipped > in > the new orientation, then I revert back to the initial arrow position. Note > that toggling the arrow location will not result in a flicker of the popup - > setting this value does not post an immediate redraw: It only changes how > the > bounds calculations are performed. > > The popup will not 'jump' unless that jump results in a fully unclipped > popup. > > > > > http://codereview.chromium.org/1774012/diff/18001/19001 > File chrome/browser/extensions/extension_popup_api.cc (right): > > http://codereview.chromium.org/1774012/diff/18001/19001#newcode159 > chrome/browser/extensions/extension_popup_api.cc:159: if > (popup_rect.IsEmpty()) > On 2010/05/17 19:11:34, rafaelw wrote: >> >> please note on the comment above the declaration of > > ExtensionPopupResized (in >> >> the interface) that it may be called with empty size (and why). > > Done. > > http://codereview.chromium.org/1774012/diff/18001/19002 > File chrome/browser/views/extensions/extension_popup.cc (right): > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode51 > chrome/browser/views/extensions/extension_popup.cc:51: } // unnamed > namespace > On 2010/05/17 19:11:34, rafaelw wrote: >> >> nit: just "namespace" > > Done. > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode185 > chrome/browser/views/extensions/extension_popup.cc:185: // space, so > convert the screen-space bounds back to view space. > On 2010/05/17 19:11:34, rafaelw wrote: >> >> This comment is unnecessary. > > Done. > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode312 > chrome/browser/views/extensions/extension_popup.cc:312: // Convert the > relative position to screen coordinates. > On 2010/05/17 19:11:34, rafaelw wrote: >> >> also unnecessary. > > Done. > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode333 > chrome/browser/views/extensions/extension_popup.cc:333: int y = > relative_rect.bottom(); > On 2010/05/17 19:11:34, rafaelw wrote: >> >> nit: keep the the if { } else {} structure here. (don't assign, then > > re-assign >> >> on if) > > Done. > > http://codereview.chromium.org/1774012/show >
On 2010/05/18 00:24:02, rafaelw1 wrote: > What I'm suggesting is that you shouldn't need to *try* repositioning > the anchor and then check to see if it improved things. You should be > able to calculate the optimum position and set it if it isn't the > current position. I changed the code to remove the brittleness on the assignment of the arrow location. I added routines that allow for the computation of the bounds of the popup based on a provided set of parameters. > > Checking to see if the intersection rect is different in width or > height from the view rect is brittle, for instance, in the case that > the rect is entirely off the screen, or if the rect is larger than the > screen in one dimension (and spanning it). You're right that testing the width/height will return an arrow location that may not be useful if the popup is clipped at both the right and left edges of the screen (or the top and bottom of the screen). Because this routine only considers repositioning of the anchor position (and not compression/projection of the popup onto the bounds of the monitor), there is no flipping operation that can result in an unclipped popup when the popup spans the monitor in one or more dimensions. The Rect::Contains check will verify this and no action will be taken. I also added an explicit case for when the popup is entirely out of the bounds of the monitor - take no action. > > On Mon, May 17, 2010 at 2:00 PM, <mailto:twiz@chromium.org> wrote: > > Thanks for your review, Rafael. > > > > I made all of the changes you suggested. > > > > As for the brittleness of the repositioning logic, my current approach is > > similar to what you suggest: I first determine if the popup is clipped by > > the > > screen, and then toggle the arrow location. If the popup is still clipped > > in > > the new orientation, then I revert back to the initial arrow position. Note > > that toggling the arrow location will not result in a flicker of the popup - > > setting this value does not post an immediate redraw: It only changes how > > the > > bounds calculations are performed. > > > > The popup will not 'jump' unless that jump results in a fully unclipped > > popup. > > > > > > > > > > http://codereview.chromium.org/1774012/diff/18001/19001 > > File chrome/browser/extensions/extension_popup_api.cc (right): > > > > http://codereview.chromium.org/1774012/diff/18001/19001#newcode159 > > chrome/browser/extensions/extension_popup_api.cc:159: if > > (popup_rect.IsEmpty()) > > On 2010/05/17 19:11:34, rafaelw wrote: > >> > >> please note on the comment above the declaration of > > > > ExtensionPopupResized (in > >> > >> the interface) that it may be called with empty size (and why). > > > > Done. > > > > http://codereview.chromium.org/1774012/diff/18001/19002 > > File chrome/browser/views/extensions/extension_popup.cc (right): > > > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode51 > > chrome/browser/views/extensions/extension_popup.cc:51: } // unnamed > > namespace > > On 2010/05/17 19:11:34, rafaelw wrote: > >> > >> nit: just "namespace" > > > > Done. > > > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode185 > > chrome/browser/views/extensions/extension_popup.cc:185: // space, so > > convert the screen-space bounds back to view space. > > On 2010/05/17 19:11:34, rafaelw wrote: > >> > >> This comment is unnecessary. > > > > Done. > > > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode312 > > chrome/browser/views/extensions/extension_popup.cc:312: // Convert the > > relative position to screen coordinates. > > On 2010/05/17 19:11:34, rafaelw wrote: > >> > >> also unnecessary. > > > > Done. > > > > http://codereview.chromium.org/1774012/diff/18001/19002#newcode333 > > chrome/browser/views/extensions/extension_popup.cc:333: int y = > > relative_rect.bottom(); > > On 2010/05/17 19:11:34, rafaelw wrote: > >> > >> nit: keep the the if { } else {} structure here. (don't assign, then > > > > re-assign > >> > >> on if) > > > > Done. > > > > http://codereview.chromium.org/1774012/show > > >
Now that we're agreed that the extension_popup_api is tblf-specific, I'm going to ask that you revert to patch set 6 (before the change to bubble_border), and I'll LGTM the changes to extension_popup.h/cc. Additionally, I've added Joi as a reviewer, and I'll let him take over on the rest from there. I'll continue to watch changes to extension code, but step out of reviewing extension_popup_api.h/cc. Note that at some point, we may want to reconsider the name of those files and api. R http://codereview.chromium.org/1774012/diff/38001/39005 File chrome/browser/views/extensions/extension_popup.h (right): http://codereview.chromium.org/1774012/diff/38001/39005#newcode48 chrome/browser/views/extensions/extension_popup.h:48: // an empty bounds, if a popup is repositioned before the hosted content nit: no comma neccessary
Reverted to the patch indicated by Rafael. Please review at your leisure.
lgtm w/ nit on extension_popup.h/cc http://codereview.chromium.org/1774012/diff/45001/24003 File chrome/browser/views/extensions/extension_popup.cc (right): http://codereview.chromium.org/1774012/diff/45001/24003#newcode331 chrome/browser/views/extensions/extension_popup.cc:331: if (BubbleBorder::is_arrow_on_top(anchor_position_)) { nit: don't need curly braces here & below.
http://codereview.chromium.org/1774012/diff/18001/19001 File chrome/browser/extensions/extension_popup_api.cc (right): http://codereview.chromium.org/1774012/diff/18001/19001#newcode159 chrome/browser/extensions/extension_popup_api.cc:159: if (popup_rect.IsEmpty()) On 2010/05/17 19:11:34, rafaelw wrote: > please note on the comment above the declaration of ExtensionPopupResized (in > the interface) that it may be called with empty size (and why). Done. http://codereview.chromium.org/1774012/diff/45001/24003 File chrome/browser/views/extensions/extension_popup.cc (right): http://codereview.chromium.org/1774012/diff/45001/24003#newcode331 chrome/browser/views/extensions/extension_popup.cc:331: if (BubbleBorder::is_arrow_on_top(anchor_position_)) { On 2010/05/19 19:47:55, rafaelw wrote: > nit: don't need curly braces here & below. Done.
LGTM w/comment nit http://codereview.chromium.org/1774012/diff/34002/47002 File chrome/browser/extensions/extension_popup_api.cc (right): http://codereview.chromium.org/1774012/diff/34002/47002#newcode182 chrome/browser/extensions/extension_popup_api.cc:182: popup->SetArrowPosition(flipped_location); Setting a position here and then resetting it a few lines below seems potentially error prone; as an informed guess I suppose as long as you do both before returning execution back to the message loop it's fine - perhaps add a comment explaining this to help avoid breakage in the future.
http://codereview.chromium.org/1774012/diff/34002/47002 File chrome/browser/extensions/extension_popup_api.cc (right): http://codereview.chromium.org/1774012/diff/34002/47002#newcode182 chrome/browser/extensions/extension_popup_api.cc:182: popup->SetArrowPosition(flipped_location); On 2010/05/19 20:59:06, Jói wrote: > Setting a position here and then resetting it a few lines below seems > potentially error prone; as an informed guess I suppose as long as you do both > before returning execution back to the message loop it's fine - perhaps add a > comment explaining this to help avoid breakage in the future. You're correct. Rafael and I discussed this previously in the review thread. The problem before was that there is no way to query the potentially new bounds of the popup/bubble border without assigning the arrow. The change I reverted had exposed 'compute bounds for a given arrow location' functionality, but I had to modify the browser-bubble code to do so. I added a comment remarking that assignment of the arrow location does not produce a draw-request.
LGTM thanks On Wed, May 19, 2010 at 6:09 PM, <twiz@google.com> wrote: > > http://codereview.chromium.org/1774012/diff/34002/47002 > File chrome/browser/extensions/extension_popup_api.cc (right): > > http://codereview.chromium.org/1774012/diff/34002/47002#newcode182 > chrome/browser/extensions/extension_popup_api.cc:182: > popup->SetArrowPosition(flipped_location); > On 2010/05/19 20:59:06, Jói wrote: >> >> Setting a position here and then resetting it a few lines below seems >> potentially error prone; as an informed guess I suppose as long as you > > do both >> >> before returning execution back to the message loop it's fine - > > perhaps add a >> >> comment explaining this to help avoid breakage in the future. > > You're correct. Rafael and I discussed this previously in the review > thread. The problem before was that there is no way to query the > potentially new bounds of the popup/bubble border without assigning the > arrow. The change I reverted had exposed 'compute bounds for a given > arrow location' functionality, but I had to modify the browser-bubble > code to do so. > > I added a comment remarking that assignment of the arrow location does > not produce a draw-request. > > http://codereview.chromium.org/1774012/show > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
