Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve visual height and vertical alignment of caret #6

Merged
merged 5 commits into from Sep 30, 2013

Conversation

peitschie
Copy link
Contributor

(Migrated from https://gitorious.org/webodf/webodf/merge_requests/170)

REVIEWER NOTE: This hasn't been tested in a collaborative setting, though I've attempted to handle the remote cursor scenario as well.

Before and after screenshots showing the differences: https://docs.google.com/file/d/0B1EH5OPb-RrjY1prck93WENtM3c/edit?usp=sharing

Implementation makes judicious use of the cursor's selection and
getClientRects to compute small adjustments necessary to make the
caret more closely match the browser caret positioning.

At the moment, each cursor must be re-fixed after any operation that
may affect the line height of the caret (e.g., removing a text char may
place cursor next to a larger text character)

This implementation has been tested with

  • Forward & backward selections
  • Wrapped text & white space
  • Image deletion

@adityab
Copy link
Member

adityab commented Sep 24, 2013

Other than that, in all this is a welcome improvement. I'll be happy to land this once the above two things are addressed. :)

@peitschie
Copy link
Contributor Author

I've addressed all the feedback here (though, it seems push -f destroys the comments).

One thing to note, I simply updated the MIN_CARET_HEIGHT_PX to a new hardcoded value, and added comments detailing where this value comes from. There is no conversion utility in master at the moment that I can find, and it seems overkill to make this a non-const for this purpose. I think the comment and renamed variable should hopefully clear it up all up :)

Implementation makes judicious use of the cursor's selection and
getClientRects to compute small adjustments necessary to make the
caret more closely match the browser caret positioning.

At the moment, each cursor must be re-fixed after any operation that
may affect the line height of the caret (e.g., removing a text char may
place cursor next to a larger text character)

This implementation has been tested with
- Forward & backward selections
- Wrapped text & white space
- Image deletion
@peitschie
Copy link
Contributor Author

Rebased on current master

adityab added a commit that referenced this pull request Sep 30, 2013
Improve visual height and vertical alignment of caret
@adityab adityab merged commit 885e99a into webodf:master Sep 30, 2013
@adityab
Copy link
Member

adityab commented Sep 30, 2013

Thanks for this. Landed!

@thz
Copy link
Member

thz commented Oct 14, 2013

retest this please

@thz
Copy link
Member

thz commented Oct 14, 2013

this PR brings a regression regarding vertical caret position/size when zoomed in (zoomlevel > 100%)
check it out at: http://www.webodf.org/demo/ci/webodf-0.4.2-1161-g885e99a/

firefox (caret position is vertically off / too high):
pr6-issue-ff

chrome (caret is too big):
pr6-issue-chrome

vandenoever pushed a commit that referenced this pull request Feb 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants