Skip to content

Commit

Permalink
Fixes #34, adjusts inline style for all text containers
Browse files Browse the repository at this point in the history
  • Loading branch information
@ryanackley
ryanackley committed Sep 16, 2014
1 parent e3d096d commit 259126f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 2 deletions.
35 changes: 35 additions & 0 deletions js/helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,41 @@ ReadiumSDK.Helpers.Rect.fromElement = function($element) {
return new ReadiumSDK.Helpers.Rect(offsetLeft, offsetTop, offsetWidth, offsetHeight);
};

ReadiumSDK.Helpers.UpdateHtmlFontSize = function($epubHtml, fontSize){
var factor = fontSize/100;
var win = $epubHtml[0].ownerDocument.defaultView;
var $textblocks = $('p, div, span', $epubHtml);

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Sep 17, 2014

Member

I was testing "accessible epub3" from Readium's bundled EPUB samples, unfortunately the text of HTML headings is not resizing because the "h" markup does not have "p, div, span" descendants or ancestors. Same problem with list items ("li") in other test books. I guess there are potentially many more use-cases, due to the great variety of HTML5 elements that do not necessarily have "p, div, span" descendants or ancestors.
Perhaps we should add to the "$textblock" list the most commonly-expected elements (headings, list items, blockquote, etc.), and add the font-size="percent%" to the HTML/BODY root (just as before). This way, the font-size="pixel value" remains forced on the descendants scanned here, and the other elements have a chance to adjust too (unless of course the original authored value is fixed pixel unit, but that would be a known caveat). Alternatively, the "$textblock" list could contain all possible element names (* wildcard)...not that I am recommending this though! :(

This comment has been minimized.

Sign in to view

Copy link
@ryanackley

ryanackley Sep 17, 2014

Author Contributor

This is intentional. Wasn't sure if headings should be resized since this isn't really detailed in the spec. It's trivial to add them to the selector.

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Sep 17, 2014

Member

I see. From an accessibility perspective, the intent of users who increase the font size is that the entire book (textual) content is scaled up.


// need to do two passes because it is possible to have nested text blocks.
// If you change the font size of the parent this will then create an inaccurate
// font size for any children.
for (var i = 0; i < $textblocks.length; i++){
var ele = $textblocks[i],
fontSizeAttr = ele.getAttribute('data-original-font-size');

if (!fontSizeAttr){
var style = win.getComputedStyle(ele);
var originalFontSize = parseInt(style.fontSize);
var originalLineHeight = parseInt(style.lineHeight);

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Sep 17, 2014

Member

Thanks Ryan! I do like the idea of scaling computed line-height values proportionally to the user-defined font-size factor (even though authored values may be not necessarily be "pixel", but also "number" or "percentage"...the end result is the same, perfectly consistent)

Just a note:
Although we do not currently explicitly handle the "normal" line-height value (resulting in parseInt=NaN), I think that the end result is acceptable (i.e. no attempt to force a scaled value of the original line-height). The "normal" value is notoriously inconsistent depending on font-family, etc., so it makes sense to refrain from mapping "normal" to let's say "1.2" (see: http://meyerweb.com/eric/css/tests/line-height/inspect-multi.html )

For the sake of code clarity though, should we explicitly handle "normal"?

This comment has been minimized.

Sign in to view

Copy link
@ryanackley

ryanackley Sep 17, 2014

Author Contributor

line-height scaling is necessary to reproduce the same results as setting the font-size percentage on the body. Otherwise, just setting the font-size will look strange because the line-height stays the same but the font-size gets larger.

Didn't realize that a 'normal' line-height gets returned in the computed style. I tried the other types of units and they all seem to return pixel values in Chrome. I agree that this case should be handled.

This comment has been minimized.

Sign in to view

Copy link
@ryanackley

ryanackley Sep 17, 2014

Author Contributor

Also, note that line-height 'normal' seems to auto-scale.
ele.setAttribute('data-original-font-size', originalFontSize);
ele.setAttribute('data-original-line-height', originalLineHeight)
}
}

for (var i = 0; i < $textblocks.length; i++){
var ele = $textblocks[i],
fontSizeAttr = ele.getAttribute('data-original-font-size'),
lineHeightAttr = ele.getAttribute('data-original-line-height'),
originalFontSize = Number(fontSizeAttr),
originalLineHeight = Number(lineHeightAttr);

ele.style.fontSize = (originalFontSize * factor) + 'px';
ele.style.lineHeight = (originalLineHeight * factor) + 'px';

}
}


/**
* @return {string}
Expand Down
2 changes: 1 addition & 1 deletion js/views/one_page_view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ ReadiumSDK.Views.OnePageView = function(options, classes, enableBookStyleOverrid
if (!_enableBookStyleOverrides) return;

if(_$epubHtml && _viewSettings) {
_$epubHtml.css("font-size", _viewSettings.fontSize + "%");
ReadiumSDK.Helpers.UpdateHtmlFontSize(_$epubHtml, _viewSettings.fontSize);
}
}

Expand Down
2 changes: 1 addition & 1 deletion js/views/reflowable_view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ ReadiumSDK.Views.ReflowableView = function(options, reader){
function updateHtmlFontSize() {

if(_$epubHtml) {
_$epubHtml.css("font-size", _fontSize + "%");
ReadiumSDK.Helpers.UpdateHtmlFontSize(_$epubHtml, _fontSize);
}
}

Expand Down

0 comments on commit 259126f

Please sign in to comment.