Bugzilla – Full Text Bug Listing
| Summary: | Text selection flickers / goes away when the mouse pointer goes outside of the view | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Mihai Sucan <mihai.sucan> |
| Component: | Editor | Assignee: | Silenio Quarti <Silenio_Quarti> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | eclipse.felipe, mihai.sucan, Silenio_Quarti |
| Version: | unspecified | ||
| Target Milestone: | 0.4 RC1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Mihai Sucan
Forgot to mention this is also a P1 bug in our Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=717631 Here's the proposed patch: https://github.com/mihaisucan/orion.client/commit/69f5b8422f36d564a3a1010f6ac64d0376ce0a27 Problem explanation: the autoScroll function, based on a timer, keeps decrementing/incrementing the x offset of the desired caret location, going towards the left. This achieves the left/right scrolling when needed and it moves the caret and the selection is extended properly. However, when the mouse goes outside the view, outside of the left edge the desired caret location is never found (of course, since it's an X offset outside of the view). Nonetheless, getXToOffset() is called with an offset that goes outside the view. There's code to cope with that (if x < lineRect.left), but there's a bug in the while (lineChild) loop. If the desired x offset is never found withing the line children rects offset is still incremented with the nodeLength, even if the x offset is far on the left. So, instead of keeping offset = lineStart ... the offset is incremented each time and the end effect is that offset becomes lineEnd. This is why the selection keeps animating/flickering because it loops between the start and end line offsets, on the autoscroll timer function. I added a check to the code in the while loop to avoid incrementing offset if it's not needed. This seems to work for me. Please review - I might have missed something! Thank you! The code clamps the x value to always be within the bounding rect of the parent (child). The fact that the 'while loop' is not finding a child that contains the x location suggests there is an inconsistency between child.getBoundingClientRect() and lineChild.getClientRects() Note that the method was coded this way (always checking for inclusion:rect.left <= x && x < rect.right) to support bidi reordering. The patch you sent will probably break for bidi. is there any padding in the child element (the line div) ? Mihai, test if this code fixes the problem:
Replace the current implementation of _getLineBoundingClientRect() with:
_getLineBoundingClientRect: function (child) {
var rect = child.getBoundingClientRect();
rect = {left: Number.MAX_VALUE, top: rect.top, right: Number.MIN_VALUE, bottom: rect.bottom};
var lineChild = child.firstChild, childRect;
while (lineChild) {
if (lineChild.ignoreChars !== lineChild.firstChild.length) {
childRect = lineChild.getBoundingClientRect();
if (childRect.left < rect.left) { rect.left = childRect.left; }
if (childRect.right > rect.right) { rect.right = childRect.right; }
}
lineChild = lineChild.nextSibling;
}
return rect;
}
-
This code probably has bad performance, but it should help us to understand the problem.
(In reply to comment #4) > Mihai, test if this code fixes the problem: > > Replace the current implementation of _getLineBoundingClientRect() with: > _getLineBoundingClientRect: function (child) { > > [...] > This code probably has bad performance, but it should help us to understand the > problem. Tested. It works! I remember I also had a problem with this method when I was fixing / looking into some older bug. I am not sure what I did back then, but IIRC I did avoid making changes to this very low level method. Do you have any ideas on how we can get this fixed? What is the difference between the rect returned by the hacked version of _getLineBoundingClientRect() and the committed one? Is it more than one pixel? (In reply to comment #6) > What is the difference between the rect returned by the hacked version of > _getLineBoundingClientRect() and the committed one? Is it more than one pixel? Yes, more than one pixel: _getLineBoundingClientRect_old l 25 t 103 r 573 b 120 _getLineBoundingClientRect l 29 t 103 r 573 b 120 When I select and move the mouse where there are no lines (eg. if I have only 3 lines in the document and I move the mouse towards the bottom of the view), I get: _getLineBoundingClientRect_old l 25 t 154 r 25 b 171 _getLineBoundingClientRect l 1.7976931348623157e+308 t 154 r 5e-324 b 171 Selection rendering changed a bit: http://img.i7m.de/show/u0hik.png Notice a few missing pixels at the start of the selection. Also notice the same number of selected pixels at the end of the selection. (In reply to comment #7) That huge difference that you see in an empty line is just bug in the new code... Now, that 4 pixels looks like padding in the line div. Would you be able to verify is there is any padding in the element (using firebug for example) ? If you do wish to have padding around the content use the .view class in the css file. (In reply to comment #8) > (In reply to comment #7) > > That huge difference that you see in an empty line is just bug in the new > code... > > Now, that 4 pixels looks like padding in the line div. Would you be able to > verify is there is any padding in the element (using firebug for example) ? > > If you do wish to have padding around the content use the .view class in the > css file. Checked. Yes, that is correct. If I remove the padding I have... the selection no longer flickers and this change fixes this bug. Thank you! Now the reason we did add padding to each line was because the selection is rendered a few pixels to the right from the line gutter, which looks weird. What do you suggest we do? > Now the reason we did add padding to each line was because the selection is
> rendered a few pixels to the right from the line gutter, which looks weird.
> What do you suggest we do?
The only element the code is ready to handle padding is the viewDiv, define a "view" class specifying the desired padding in your css file that is passed to the constructor of the TextView.
See TextView#_getFrameHTML() (to see the default values) and org.eclipse.orion.client.editor/web/orion/textview/textview.css (currently it only sets the background to white).
(In reply to comment #10) > > Now the reason we did add padding to each line was because the selection is > > rendered a few pixels to the right from the line gutter, which looks weird. > > What do you suggest we do? > > The only element the code is ready to handle padding is the viewDiv, define a > "view" class specifying the desired padding in your css file that is passed to > the constructor of the TextView. I did that, .view { paddling-left: 4px }. But this pushes the text selection as well. This is why I was asking... (In reply to comment #11) > I did that, .view { paddling-left: 4px }. But this pushes the text selection as > well. This is why I was asking... I'm sorry, I missed that. It seems that we would have to fix our hit test to respect padding in the lineDivs? Silenio, do you see an alternative ? http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=87adb02b69e2ba6f68942fd3326927378d086b31 Mihai, try this out. We are trying to support padding and border in the lineDiv element. Let me know if it works for you. Thanks. (In reply to comment #13) > http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=87adb02b69e2ba6f68942fd3326927378d086b31 > > Mihai, try this out. We are trying to support padding and border in the lineDiv > element. Let me know if it works for you. Thanks. How risky is the code change? I'd prefer to use padding-left on .view rather than take a patch now that might cause other regressions. I'm "late" in my process of updating the source editor for Firefox and I'd like to cherry-pick only safe changes now. If you consider it's "risky" then I'll take this later, in a future Orion update. Thank you very much for your work - Silenio and Felipe! |
