Commit 7b01a6c8 authored by Robert Knight's avatar Robert Knight

Fix margins on annotation body content causing content to be cropped

Fix an issue where elements in the annotation body that were styled with
margins, such as headings and block quotes, could cause the bottom of an
excerpt to cropped.

The issue was that margin collapsing [2] caused the margin to "leak" out of
the excerpt's content `<div>` and push it vertically away from the outermost
element of the excerpt. The `max-height` applied to the outermost
element took into account the content's _height_ but not its vertical
offset.

The fix here is to make the excerpt content div a new block-formatting
context [1] which ensures that any margins on elements inside the
content are placed _inside_ the content element. There are various ways
of doing this, but `display: inline-block` is backwards compatible down
to IE 11.

 - Fix margin collapsing issue by making content a new block formatting
   context
 - Remove unused `excerpt__container` class and an incorrect comment
   about the excerpt class
 - Remove an unnecessary `test-name` attribute on the content element and
   use the class name to find it in tests instead
 - Add a note about some refactoring that can be done post SASS-module
   adoption

[1] https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Block_formatting_context
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Model/Mastering_margin_collapsing

Fixes #1518
parent 27cd90ef
...@@ -115,7 +115,7 @@ function Excerpt({ ...@@ -115,7 +115,7 @@ function Excerpt({
return ( return (
<div className="excerpt" style={contentStyle}> <div className="excerpt" style={contentStyle}>
<div test-name="excerpt-content" ref={contentElement}> <div className="excerpt__content" ref={contentElement}>
{children} {children}
</div> </div>
<div <div
......
...@@ -53,7 +53,7 @@ describe('Excerpt', () => { ...@@ -53,7 +53,7 @@ describe('Excerpt', () => {
it('renders content in container', () => { it('renders content in container', () => {
const wrapper = createExcerpt(); const wrapper = createExcerpt();
const contentEl = wrapper.find('[test-name="excerpt-content"]'); const contentEl = wrapper.find('.excerpt__content');
assert.include(contentEl.html(), 'default content'); assert.include(contentEl.html(), 'default content');
}); });
......
// FIXME - Remove the `@at-root` here when SASS modules are used, as local
// variables will no longer be exposed to other modules.
@at-root { @at-root {
$expand-duration: 0.15s; $expand-duration: 0.15s;
// the truncated body of the <excerpt>
.excerpt { .excerpt {
transition: max-height $expand-duration ease-in; transition: max-height $expand-duration ease-in;
overflow: hidden; overflow: hidden;
position: relative; position: relative;
} }
// a container which wraps the <excerpt> and contains the excerpt .excerpt__content {
// itself plus the shadow at the bottom that can be clicked to expand or // Create a new block-formatting context. This prevents any margins on
// collapse it // elements inside the excerpt from "leaking out" due to margin-collapsing,
.excerpt__container { // which would push this container element away from the top of the excerpt.
position: relative; //
// See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Block_formatting_context
// and https://github.com/hypothesis/client/issues/1518.
display: inline-block;
} }
// inline controls for expanding and collapsing // inline controls for expanding and collapsing
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment