Replace font loader with a resize event in reflow and scroll view #308
Conversation
|
Tested on iOS, OS X, Chrome, Firefox and Edge. |
| function(Globals, $, _, EventEmitter, BookmarkData, CfiNavigationLogic, | ||
| CurrentPagesInfo, Helpers, PageOpenRequest, | ||
| ViewerSettings, FontLoader) { | ||
| ViewerSettings, ResizeSensor) { | ||
| /** |
There was a problem hiding this comment.
In this PR, the removal of FontLoader (native and polyfill) means that we can also remove the NPM dependency, right?
https://github.com/readium/readium-shared-js/blob/develop/package.cson#L126
"FontLoader": 'smnh/FontLoader'
There was a problem hiding this comment.
Oh, and of course the class file itself can be removed: https://github.com/readium/readium-shared-js/blob/develop/js/views/font_loader.js
The bookmark is updated every time the pagination changes (in onPaginationChanged). If there's no deferred page request, at the end of updatePagination, openPage is called with the bookmark's CFI so whenever viewport is resized or a font is changed or loaded and the pagination changes, the reader stays on the same page. In ReaderView, after a viewport resize, the reader doesn't try to jump to the bookmark saved at the start of the resize, as it is now handled directly by the ReflowableView.
… load or viewport resize
|
@danielweck Indeed, I removed the I mentioned in a call that I was concerned that any change of the size, triggered by a change of the font size or the font family, or by any other cause actually, could push the visible page out the screen. To prevent this, I think we need to proactively keep a bookmark to the last known position, updated every time the user goes to a previous or next page, to a bookmark or to a target of the table of contents. I implemented this approach in the ReflowView ( Last point, I implemented the resize sensor in the ScrollView (see #312). I removed the method |
olivierkorner
changed the title
|
@olivierkorner I think this is a good change to make anyway. |
|
TODO: ResizeSensor throttle or debounce to avoid flooding the event listeners that (or example) re-layout the stack of iframes in the continuous scroll view. |
|
TODO: check that Media Overlays playback pause/resume behaviour is not affected by different CFI bookmark logic. |
|
TODO: check that the CFI current reading location does not drift when restoring following a change of font size, etc. Top-left corner (first visible CFI) cause drift, better to preserve the original CFI even it is now located in the "middle" of a page spread. |
|
TODO: extract the "resize sensor" JS code as an NPM dependency: Let's also consider this optimized version (still uses scroll detection, as based on the above JS lib): Just for reference, these alternatives are getting old now (unmaintained), compared to the above: |
|
@danielweck Actually, the ResizeSensor class in the PR is slightly different from the one in marcj/css-element-queries. I had to fix it a flaw that would cause some function to be called repeatedly after a resize has occured. |
|
Great, so let's fork https://github.com/marcj/css-element-queries/blob/master/src/ResizeSensor.js (at https://github.com/danielweck/css-element-queries to follow the current convention), and let's add this new NPM dependency to: "css-element-queries": 'danielweck/css-element-queries'...and then let's configure the RequireJS config: ResizeSensor:
process._RJS_rootDir(1) + '/node_modules/css-element-queries/src/ResizeSensor.js',...and of course change the RequireJS function signatures (dependency injection) from |
Whenever openPage() is called in ReflowView, the pageRequest is saved. After content is resized (font has changed or viewport has been resized), we go back to the saved pageRequest. If the user goes to next or previous page, the saved pageRequest is ditched and we save the current position in a new pageRequest.
|
Thanks @olivierkorner for extracting the NPM dependency. I logged a global issue here: readium/readium-js-viewer#565 (comment) |
|
Resize Sensor diff in our fork: marcj/css-element-queries@master...olivierkorner:master |
|
ToDo: test "dynamic content sizes". EDIT: done. Works just fine in reflow and scroll views. |
This pull request is a Work In Progress
Related issue(s) and/or pull request(s)
#304
#300
Additional information
This is intended as a fix for the font loader that doesn't as expected when there's no support for a native font loading API. But it might be useful when for some reason the layout of the chapter displayed changes (element becomes visible, remote content loaded, etc.)
Inspiration came from FontLoader itself, which watches for a
scrollevent to detect a div containing some text has been resized, due to the font substitution.The method is documented in following article:
http://www.backalleycoder.com/2013/03/18/cross-browser-event-based-element-resize-detection/
Applied here, the idea is to detect when the size of the
bodyelement in a reflowable iframe has changed and when this happens, to trigger an update of the pagination by callingupdatePagination.Some optimization is probably possible, since
updatePaginationis called anyway when new settings are applied or when the viewport is resized.Feeback welcome!