Last modified: 2014-06-17 20:32:04 UTC
Created attachment 14997 [details] Screenshot of excessive whitespace on top of an in-content dialog with h2 heading Since inner padding of the dialog container and top margin of the header don't collapse, they get stacked and now have an excessive amount of whitespace (see screenshot). The margin-top of 'div#content h2' should be safe to remove. We didn't have it in the past, and in any regular text flow context it is already spaced away from the preceding element by the bottom margin of block level elements (like paragraphs and tables).
Change 123024 had a related patch set uploaded by Krinkle: vector: Remove margin-top from 'div#content h2' https://gerrit.wikimedia.org/r/123024
I think this is probably something where special UI elements like dialogs should be adding/changing their own H2 styles, rather than reusing styles intended for content pages. Repeating Kaldari's comment on the patch: "The 1em margin was implemented on purpose by the design team. As mentioned in the FAQ, they wanted to significantly increase the whitespace between sections. Unfortunately on desktop we don't have any "section" DOM element, so adding top margin to h2s is the best way to accomplish that. Since dialogs don't often belong to the content div, it seems like better solutions to bug 63390 would be: 1. Move the dialog outside the content div; 2. Don't use h2s in that dialog, or... 3. Override the h2 style in that case. Is there more than 1 dialog affected by this bug?"
I think you might've missed that I said the margin-top can be safely removed because it is ineffective. It isn't "increasing" the visible whitespace between sections, because all block level elements already have sufficient *bottom* margin. And browsers collapse/merge the top margin of the next element with the bottom margin of the previous element. No matter whether it's a paragraph, list, <pre>, gallery or <table>, their bottom margin is already taking care of that space. When I removed the top-margin of headings locally it made little to no visible difference. So if the goal was to increase the space between sections, it failed to do that. If we do want to increase space between sections, I suggest it is implemented in different way. Using margin-top for that is in my opinion inappropriate and bound to cause layout hazards. I agree the specific instance in which I ran into this bug is dubious, but in general you mustn't assume that a heading will only be used in the root flow of the content and only in the n+1th section of page. Using margin-top means that: * Using the heading in the first section on a page will cause a weird gap on top because it's trying to offset itself from the previous section when in fact that is the top of the page. Which is a problem since content wrappers already have padding, and that doesn't merge with margins of elements inside. * Using the heading inside another element (blockquote, table, infobox, form), even inside elements that aren't intended as their own block (e.g. heading bubbles such as on [1]) will now have a margin on top on the inside intended to separate itself from a section. This could in theory be worked around by resetting margins or creating negative catch margins. However that would be an unintuitive hack that doesn't scale because it's moving the responsibility to the wrong component (e.g. if you want to use a <table> on a page, you cannot be expected to have to set all kinds of inline styles to make it render properly, it is the skin's fault for changing the default of all headings to act like section separators. The skin should take care to recognise the relevant headings and style them and leave the others untouched). I'm not going to spend time on finding one of many alternate ways to implement section separation margins on desktop, but the way it is implemented right now is and will continue to cause all kinds of layout problems due to it applying top-margins that are too large when applied in a context that isn't between two sections. [1] https://www.mediawiki.org/w/index.php?title=Help:VisualEditor/User_guide&oldid=984581#toc)
Rephrasing bug to reflect the two most common problem cases: * Using a heading as first element on page (e.g. the first section, as opposed to between two sections). * Using a heading inside a container that isn't the root of the page (e.g. inside a bubble such as on [1], or any other kind of sub container). [1] https://www.mediawiki.org/w/index.php?title=Help:VisualEditor/User_guide&oldid=984581#toc
>I think you might've missed that I said the margin-top can be safely removed >because it is ineffective. It isn't "increasing" the visible whitespace between >sections... I don't think that's accurate. Having "h2 { margin-top: 1em; }" is definitely increasing the whitespace significantly between sections. With that rule in place, the whitespace is effectively 21 pixels (0.875em x 1.5em x 1em x 16px). Without it, in most cases, the whitespace is only 7 pixels (0.875em x 0.5em x 16px). That's a big difference. If we want to change the amount of whitespace between sections, we need to get input from design on this. If we don't want to change the amount of whitespace between sections, we need to find a different way to implement the whitespace. Currently, however, I don't see any better way to implement the whitespace between sections. As the bug is currently defined, I recommend WONTFIX.
Because the discussion here clearly points to a lack of consensus, I'm removing the 1.23.0 target.
Change 123024 abandoned by Krinkle: vector: Remove margin-top from 'div#content h2' Reason: Implementing something in a hacky way that breaks expectations about how page layout works and then putting the burden on other maintainers to make it work well with the rest of the web is imho not ideal practice. But I can't invest in this now, so snafu. You're welcome. https://gerrit.wikimedia.org/r/123024
CCing Jared since this is really a design issue.
This is as designed, as Steven said, if there are 1 one off cases where the padding is too large within specific features or use cases (not articles or content pages) the feature teams should write modifications to the style for those cases.
(In reply to Jared Zimmerman (WMF) from comment #9) > This is as designed, as Steven said, if there are 1 one off cases where the > padding is too large within specific features or use cases (not articles or > content pages) the feature teams should write modifications to the style for > those cases. This is a complaint about the breakage of unmaintained code that was written with the expectation that people wouldn't unexpectedly change these, though. There are no "feature teams" to fix the breakage for you…
What James says is true, although there currently isn't any way to have large margins between the sections on desktop without increasing the margins on ".content h2", so unless design agrees to reduce those margins, the fix will have to be to the unmaintained code (at least until the desktop DOM includes some sort of section elements).
(In reply to Ryan Kaldari from comment #11) > What James says is true, although there currently isn't any way to have > large margins between the sections on desktop without increasing the margins > on ".content h2", so unless design agrees to reduce those margins, the fix > will have to be to the unmaintained code (at least until the desktop DOM > includes some sort of section elements). Could we tweak it to have a !firstChild or something on H2s so it only triggers for the H2s that we actually want to style?
>Could we tweak it to have a !firstChild or something on H2s so it only triggers >for the H2s that we actually want to style? I can't think of any way to do this that would be reliable. We could do something like... #content h2(:not( .dialogHeader )) but that would still require making sure that all headers in dialogs included that class. (And if we're updating those headers anyway, we might as well convert them from h2s into something else.) We could also use jQuery to try to guess which h2s are actually section headers and which ones aren't, but that would cause a style change flash. I still haven't heard any evidence that this bug actually affects more than a single dialog box. If it's just one dialog, is it really that hard to fix, even though the code is unmaintained? What extension generates that dialog box?
