Issue 5598654983307264: Issue 1751 - Highlight elements by injecting an overlay (Closed)
|
Created:
Jan. 5, 2015, 3:35 p.m. by Sebastian Noack Modified:
Jan. 13, 2015, 2:22 p.m. Reviewers:
kzar CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 1751 - Highlight elements by injecting an overlay
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Rebased and addressed comments #MessagesTotal messages: 6
http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:60: element._unhighlight = function() Could this cause a memory leak as _unhighlight functions are added but never removed? http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:68: var originalBoxShadow = element.style.getPropertyValue ("box-shadow"); Why extra spacing before opening bracket? (Maybe it makes sense aesthetically when not viewed in rietveld? http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:94: var overlay; Seems confusing to me that overlay is declared here, below where it's first used inside the above functions. Could you move this declaration above? http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:95: if (!("prisoner" in element) && (overlay = addElementOverlay(element))) Could you add a comment before this if statement explaining why we sometimes need to highlight elements with a style attribute still? http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:263: overlay.setAttribute('style', 'opacity:0.4; display:inline-box; position:absolute; overflow:hidden; box-sizing:border-box;'); Are these changes relevant to the issue?
http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:60: element._unhighlight = function() On 2015/01/13 11:04:24, kzar wrote: > Could this cause a memory leak as _unhighlight functions are added but never > removed? It is actually removed, see unhighlightElement(). http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:68: var originalBoxShadow = element.style.getPropertyValue ("box-shadow"); On 2015/01/13 11:04:24, kzar wrote: > Why extra spacing before opening bracket? (Maybe it makes sense aesthetically > when not viewed in rietveld? I find it more readable, when similar code is aligned. But unfortunately, this is against our coding style guide. So I removed the redundant whitespaces. http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:94: var overlay; On 2015/01/13 11:04:24, kzar wrote: > Seems confusing to me that overlay is declared here, below where it's first used > inside the above functions. Could you move this declaration above? I moved the declaration into highlightWithOverlay(). Frankly, I didn't like that construct too. Since this method is only called on hover, the element is always rendered and addElementOverlay() won't return null. http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:95: if (!("prisoner" in element) && (overlay = addElementOverlay(element))) On 2015/01/13 11:04:24, kzar wrote: > Could you add a comment before this if statement explaining why we sometimes > need to highlight elements with a style attribute still? We only highlight elements with style attributes if they are already overlays we have created before. I think this should be more clear now.
http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:263: overlay.setAttribute('style', 'opacity:0.4; display:inline-box; position:absolute; overflow:hidden; box-sizing:border-box;'); On 2015/01/13 11:04:24, kzar wrote: > Are these changes relevant to the issue? What about these changes?
http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5598654983307264/diff/5717271485874176/incl... include.postload.js:263: overlay.setAttribute('style', 'opacity:0.4; display:inline-box; position:absolute; overflow:hidden; box-sizing:border-box;'); On 2015/01/13 13:26:34, kzar wrote: > On 2015/01/13 11:04:24, kzar wrote: > > Are these changes relevant to the issue? Kinda; it already prior to my changes here, it were a bug that the computed value of the "height" and "width" CSS properties were used, instead the actual dimensions as provided by the offsetWidth and offsetHeight. However, since overlays were only injected for images, <object> elements and in a few other cases, it usually didn't show any undesired effect. This issue became more critical now where we always use overlays to highlight elements. > What about these changes? The problem is that you can't rely on the "width" and "height" CSS properties, since they don't consider border and padding (except "box-sizing: border-box" is used). Hence the overlay would be too small.
LGTM 
|
