Fix for Adapt.scrollTo 'jump' bug #1400
Conversation
src/core/js/adapt.js
Outdated
| @@ -106,9 +106,11 @@ define([ | |||
| } | |||
|
|
|||
| var navigationHeight = $(".navigation").outerHeight(); | |||
| // prevent scroll issue when component description aria-label coincident with top of component | |||
| var ariaLabelBuffer = $(selector.is('.component')) ? 5 : 0; | |||
There was a problem hiding this comment.
5 is a magic number here.
There was a problem hiding this comment.
I know it's not ideal and we have other magic numbers with some of the deferred calls, particularly in the a11y code. There is an alternative, which I discussed with @kirsty-hames and that is to place this 5px offset in the theme to shove the aria-label down. I preferred the code method as it means we don't need to make this change in multiple themes.
@brian-learningpool, @kirsty-hames, @tomgreenfield is the consensus that this is better fixed in the theme(s)?
There was a problem hiding this comment.
If this offset is going to handled here, I'd prefer a more programmatical approach like below:
var $element = $(selector);
var offsetTop = -navigationHeight;
if (Adapt.config.get('_accessibility')._isActive &&
$element.hasClass('component')) {
offsetTop -= $element.find('.aria-label').height() || 0;
}Am I right in thinking it's only an issue in Firefox? If it's such a small use case that it's only when accessibility is active and it's a component and in one browser and in a certain style of art direction, then it may make more sense to fix in the theme.
There was a problem hiding this comment.
Present in IE*, Edge and Firefox. The above could work (amending to .aria-label:first), though I'd have to test the reported height (1px and 3px I think, respectively) would be sufficient.
There was a problem hiding this comment.
Cool, well the height() method already uses the first element if multiple are found. Haven't yet tested in IE8 or Edge but could have a gander later if you like.
There was a problem hiding this comment.
I can't actually replicate the original issue in IE8, while the above code fixes it for me in Firefox & Edge.
There was a problem hiding this comment.
Yes, it's unusual, but it seems IE8 doesn't have this problem, just IE9 and above!
This fix addresses an issue when navigating to components via page-level progress in certain circumstances. If the component description aria-label is aligned to the top of the component div (as happens in certain art directions), Adapt.scrollTo would centre the top of the component within the viewport rather than at the top of the viewport. N.B. this issue is normally only replicable when accessibility has been activated.