Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6141002:
dom-ui settings: display bubbles for sub-page search results.... (Closed)
|
Created:
9 years, 11 months ago by csilv Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Descriptiondom-ui settings: display bubbles for sub-page search results.
BUG=59267
TEST=Manual exercise search functionality.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72372
Patch Set 1 #Patch Set 2 : '' #
Total comments: 19
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 21
Patch Set 6 : '' #
Messages
Total messages: 10 (0 generated)
+jhawkins, arv for review.
Erik, James, per discussion... these bubbles may not go in. there's been some discussion that we don't want to do these, but we don't have an alternative UI. so consider holding off spending time on this review until I get a solid update.
Per email with Ben & Alex, we can proceed on this. Please review as time permits. thanks, Chris
http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/search_page.css (right): http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:11: left: -1000px; /* minor hack: position off-screen by default */ Use top: -1000px instead http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:12: top: 0px; s/0px/0/ http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:20: text-align: center; I think you should probably have pointer-events: none here so that the bubbles do not block the underlying label/control. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:23: .search-bubble:after { Could you explain what this is for? http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/search_page.js (right): http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:36: function(e) { self.updatePosition(); }, 250); setInterval(this.updatePosition.bind(this), 250); http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:63: var left = Math.floor(owner.offsetLeft + You can skip the floor here. left is not limited to integers http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:64: owner.offsetWidth/2 - this.offsetWidth/2); ws around binops http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:68: // best performance. The main slowdown is the offset* followed by setting the style. One thing you can do to improve this is to do the measuring of all bubbles first before you set the style of them. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:246: length = page.pageDiv.childNodes.length; Change this to children and loop over the children instead so that you do not have to check the nodeType http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:266: childDiv = page.pageDiv.childNodes[i]; children
http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/search_page.css (right): http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:11: left: -1000px; /* minor hack: position off-screen by default */ On 2011/01/07 22:09:45, arv wrote: > Use top: -1000px instead Done. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:12: top: 0px; On 2011/01/07 22:09:45, arv wrote: > s/0px/0/ Done. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:20: text-align: center; On 2011/01/07 22:09:45, arv wrote: > I think you should probably have pointer-events: none here so that the bubbles > do not block the underlying label/control. Done. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.css:23: .search-bubble:after { On 2011/01/07 22:09:45, arv wrote: > Could you explain what this is for? This is used to create the bubble "pointer" (arrow pointing up). http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/search_page.js (right): http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:36: function(e) { self.updatePosition(); }, 250); On 2011/01/07 22:09:45, arv wrote: > setInterval(this.updatePosition.bind(this), 250); Done. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:63: var left = Math.floor(owner.offsetLeft + On 2011/01/07 22:09:45, arv wrote: > You can skip the floor here. left is not limited to integers Done. Coming from other platforms, I was concerned about blurry edges caused by non-integer coordinates, but in a quick test it appears that the fraction will be ignored. That makes life easier. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:64: owner.offsetWidth/2 - this.offsetWidth/2); On 2011/01/07 22:09:45, arv wrote: > ws around binops Done. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:246: length = page.pageDiv.childNodes.length; On 2011/01/07 22:09:45, arv wrote: > Change this to children and loop over the children instead so that you do not > have to check the nodeType Done. Thanks for pointing this out. 'children' appears to be rather poorly documented (as far as I can find), but found it referenced on quirksmode.org. Curious if you have a good/complete reference to dom api that I should bookmark. http://codereview.chromium.org/6141002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/search_page.js:266: childDiv = page.pageDiv.childNodes[i]; On 2011/01/07 22:09:45, arv wrote: > children Done.
arv, if you can take another look at this at your convenience, I would appreciate it. it took me a while to respond, so it may have fallen down to the bottom of your list. thx!
http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... File chrome/browser/resources/options/search_page.css (right): http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:10: position: absolute; Alphabetical order please http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:12: left: -1000px; /* minor hack: position off-screen by default */ Use top instead since left is not good when using RTL http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:28: left: 50px; rtl? http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... File chrome/browser/resources/options/search_page.js (right): http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:64: var top = owner.offsetTop + owner.offsetHeight; How do we handle the case where the bubble is displayed outside the viewport? Can chrome/browser/resources/shared/js/cr/ui/position_util.js help you? http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:184: for (var i = 0; i < length; i++) { The following pattern is slightly easier on the eyes. for (var i = 0, child; child = children[i]; i++) { } but the current for loops are ok with me. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:187: if (childDiv.nodeName != 'SECTION') Use tagName instead http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:246: if (childDiv.nodeName == 'SECTION') { tagName http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:393: createSearchBubble_: function(element, text) { I would just make this a local function instead http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:395: var sibling = element.previousSibling; previousElementSibling so you can skip testing for classList http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:400: var parent = element.parentNode; parentElement http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:402: var bubble = SearchBubble(text); missing new
Sorry for being slow. I'm still in catch up mode...
http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... File chrome/browser/resources/options/search_page.css (right): http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:10: position: absolute; On 2011/01/21 23:19:31, arv wrote: > Alphabetical order please Done. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:12: left: -1000px; /* minor hack: position off-screen by default */ On 2011/01/21 23:19:31, arv wrote: > Use top instead since left is not good when using RTL You asked me to change this from top to left previously. ;-) Changed back to top. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.css:28: left: 50px; On 2011/01/21 23:19:31, arv wrote: > rtl? The pointer is centered in the bubble, so this should be fine for RTL. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... File chrome/browser/resources/options/search_page.js (right): http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:64: var top = owner.offsetTop + owner.offsetHeight; On 2011/01/21 23:19:31, arv wrote: > How do we handle the case where the bubble is displayed outside the viewport? > > Can chrome/browser/resources/shared/js/cr/ui/position_util.js help you? We don't do anything special, the bubble will be positioned off-screen. The extra logic in position_util.js is not needed here (it's more useful for popup menus/controls). The positioning logic is slightly different as well (left or right aligned, rather than centered.) http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:184: for (var i = 0; i < length; i++) { On 2011/01/21 23:19:31, arv wrote: > The following pattern is slightly easier on the eyes. > > for (var i = 0, child; child = children[i]; i++) { > > } > > but the current for loops are ok with me. Done. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:187: if (childDiv.nodeName != 'SECTION') On 2011/01/21 23:19:31, arv wrote: > Use tagName instead Done. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:246: if (childDiv.nodeName == 'SECTION') { On 2011/01/21 23:19:31, arv wrote: > tagName Done. (here and elsewhere) http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:395: var sibling = element.previousSibling; On 2011/01/21 23:19:31, arv wrote: > previousElementSibling so you can skip testing for classList Done. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:400: var parent = element.parentNode; On 2011/01/21 23:19:31, arv wrote: > parentElement Done. http://codereview.chromium.org/6141002/diff/16001/chrome/browser/resources/op... chrome/browser/resources/options/search_page.js:402: var bubble = SearchBubble(text); On 2011/01/21 23:19:31, arv wrote: > missing new Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
