Bartosz Dziewoński has uploaded a new change for review. https://gerrit.wikimedia.org/r/192240
Change subject: ve.ce.FocusableNode: Ignore children of elements using CSS column-count in Chrome ...................................................................... ve.ce.FocusableNode: Ignore children of elements using CSS column-count in Chrome Chrome incorrectly measures children of nodes with columns [1], let's just ignore them rather than render a possibly bizarre highlight. They will usually not be positioned, because Chrome also doesn't position them correctly [2] and so people avoid doing it. Of course there are other ways to render a node outside the bounding box of its parent, like negative margin. We do not handle these cases, and the highlight may not correctly cover the entire node if that happens. This can't be worked around without implementing CSS layouting logic ourselves, which is not worth it. We limit the change to Chrome and its likes by checking for support for -webkit-column-count property first (with browser prefix). I ran some crude performance measurements [3] on Firefox, IE 11 and Opera 12, and there's no noticeable change in any of them (testing on [4]). Chrome runs much faster on nodes with columns inside (understandable, as we entirely skip a lot of calculations in this case) and, which is less understandable, also slightly faster on other nodes. (Trying to unconditionally run the code in Firefox resulted in 2x performance regression, so I didn't check further.) [1] http://code.google.com/p/chromium/issues/detail?id=391271 [2] https://code.google.com/p/chromium/issues/detail?id=291616 [3] https://phabricator.wikimedia.org/P322 [4] https://en.wikipedia.org/wiki/The_Fighting_Temeraire Bug: T52036 Change-Id: I4d0fae57ccb4017b080d4769660ed6b5e176287b --- M src/ce/ve.ce.FocusableNode.js 1 file changed, 37 insertions(+), 6 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor refs/changes/40/192240/1 diff --git a/src/ce/ve.ce.FocusableNode.js b/src/ce/ve.ce.FocusableNode.js index ffde6ce..1259ce8 100644 --- a/src/ce/ve.ce.FocusableNode.js +++ b/src/ce/ve.ce.FocusableNode.js @@ -401,9 +401,10 @@ * Calculate position of highlights */ ve.ce.FocusableNode.prototype.calculateHighlights = function () { - var i, l, + var i, l, $set, rects = [], filteredRects = [], + webkitColumns = 'webkitColumnCount' in document.createElement( 'div' ).style, surfaceOffset = this.surface.getSurface().getBoundingClientRect(); function contains( rect1, rect2 ) { @@ -413,14 +414,38 @@ rect2.bottom <= rect1.bottom; } - this.$focusable.find( '*' ).addBack().each( function () { - var i, j, il, jl, contained, clientRects; + function process( el ) { + var i, j, il, jl, contained, clientRects, + $el = $( el ); - if ( $( this ).hasClass( 've-ce-noHighlight' ) ) { + if ( $el.hasClass( 've-ce-noHighlight' ) ) { return; } - clientRects = this.getClientRects(); + if ( + webkitColumns && + ( $el.css( '-webkit-column-count' ) || $el.css( '-webkit-column-size' ) ) + ) { + // Chrome incorrectly measures children of nodes with columns [1], let's + // just ignore them rather than render a possibly bizarre highlight. They + // will usually not be positioned, because Chrome also doesn't position + // them correctly [2] and so people avoid doing it. + // + // Of course there are other ways to render a node outside the bounding + // box of its parent, like negative margin. We do not handle these cases, + // and the highlight may not correctly cover the entire node if that + // happens. This can't be worked around without implementing CSS + // layouting logic ourselves, which is not worth it. + // + // [1] http://code.google.com/p/chromium/issues/detail?id=391271 + // [2] https://code.google.com/p/chromium/issues/detail?id=291616 + + // jQuery keeps nodes in its collections in document order, so the + // children have not been processed yet and can be safely removed. + $set = $set.not( $el.find( '*' ) ); + } + + clientRects = el.getClientRects(); for ( i = 0, il = clientRects.length; i < il; i++ ) { contained = false; @@ -441,7 +466,13 @@ rects.push( clientRects[i] ); } } - } ); + } + + $set = this.$focusable.find( '*' ).addBack(); + // Calling process() may change $set.length + for ( i = 0; i < $set.length; i++ ) { + process( $set[i] ); + } // Elements with a width/height of 0 return a clientRect with a width/height of 1 // As elements with an actual width/height of 1 aren't that useful anyway, just -- To view, visit https://gerrit.wikimedia.org/r/192240 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4d0fae57ccb4017b080d4769660ed6b5e176287b Gerrit-PatchSet: 1 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: wmf/1.25wmf17 Gerrit-Owner: Bartosz Dziewoński <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
