Page MenuHomePhabricator

There should be a space between span.mw-collapsible-toggle and its preceding content
Closed, DuplicatePublic

Description

For collapsible elements, there is no space in the markup, between the dynamically generated element span.mw-collapsible-toggle and the preceding content.

This is fine in most situations, because this toggle element has a CSS float by default.
But in some situations, e.g. in table captions, this CSS float is undone. In such case, the toggle element is right next to its preceding content, without spacing.

Demo:

{| class="mw-collapsible wikitable" style="width:50%"
|+ caption
! header
|-
| content
|}

Possible fix: add the following line in jquery.makeCollapsible.js:

      buildDefaultToggleLink = function () {
        return $( '<a>' )
          .attr( {
            role: 'button',
            tabindex: 0
          } )
          .text( collapseText )
          .wrap( '<span class="mw-collapsible-toggle"></span>' )
            .parent()
+++         .before(' ')
            .prepend( '<span class="mw-collapsible-bracket">[</span>' )
            .append( '<span class="mw-collapsible-bracket">]</span>' )
            .on( 'click.mw-collapsible keypress.mw-collapsible', actionHandler );
      };

Event Timeline

Probably a better fix, as it would work on more cases: add a CSS margin-left:0.3em

You can try it on mw:Manual:Collapsible_elements:

$('.mw-collapsible-toggle').css('margin-left', '0.3em');

I don't see the issue when I test the provided demo code -

Screen Shot 2017-01-16 at 1.17.00 PM.png (110×626 px, 13 KB)

I do understand what you're saying from the demo on the manual page -

Screen Shot 2017-01-16 at 11.13.22 AM.png (226×310 px, 28 KB)


I don't think this is happening because the element is not floating but because the width of the table is too small. Increasing the width shows that the float works fine-

{| class="mw-collapsible mw-collapsed wikitable" width="400"
! The header || remains visible
|-
| This  content || is hidden
|-
| at first || load time
|}

Screen Shot 2017-01-16 at 1.19.15 PM.png (105×426 px, 16 KB)

  1. About my demo code with table caption: I'm surprised you have a space. I don't have space, using Firefox and Chrome.
  2. About the "narrow tables" on the manual page: you are correct, and that's precisely why I then proposed the "better fix", adding a margin-left CSS.

About the "narrow tables" on the manual page: you are correct, and that's precisely why I then proposed the "better fix", adding a margin-left CSS.

Mind submitting a patch? I am not sure if this would be the right thing, but a patch would get you the right feedback.

Change 338045 had a related patch set uploaded (by Gerrit Patch Uploader):
Add spacing before the mw-collapsible-toggle links

https://gerrit.wikimedia.org/r/338045

I don't see the issue when I test the provided demo code -

Screen Shot 2017-01-16 at 1.17.00 PM.png (110×626 px, 13 KB)

The issue doesn't happen if you try on Special:ExpandTemplates, but it does happen if you edit a page and do "show preview".

I'm not sure about how to handle this, see the comments by Krinkle on patch set 4 on the gerrit ticket:

Confirmed bug is fixed when testing locally, however use of 'mw-collapsible-toggle' is somewhat contentious since it used both for toggles we made and ones already on the page. Example: https://test.wikipedia.org/wiki/CollapsingTestpageMw - will get an unexpected white padding between the border and the blue shaded header.

(Actually, not 100% sure; I thought custom toggles had their own class. I guess this example works by accident? Let's see how common this is.)

Od1n removed Od1n as the assignee of this task.Jul 13 2017, 3:31 AM

Changing the selector from .mw-collapsible-toggle to the narrower caption .mw-collapsible-toggle would fix the case pointed out by @Krinkle on Gerrit, and I guess would be robust enough to avoid the false positives.

Uploaded a new patch to https://gerrit.wikimedia.org/r/#/c/338045/

You will note a change from the current styles: the float is not longer applied to all .mw-collapsible-toggle but only to to caption .mw-collapsible-toggle.
I did this to keep grouped the floats and the margins I have added, but actually it is much better this way, because the float is usually undesirable on the manually added toggles.

Here is another real-life example of a manual use of .mw-collapsible-toggle:
https://en.wikipedia.org/wiki/Wikipedia:Book_sources#Libraries
You will notice that here too, inline CSS has been added to clear the float added by default.

Nope, narrowing the addition of the float was wrong.

However, I have uploaded again a new patch, this time I'm adding the margin just where the float is undone.
Note how the rules I am adding nicely complement the rules above.

Change 338045 had a related patch set uploaded (by Legoktm; owner: Vlakoff):
[mediawiki/core@master] Add spacing before the mw-collapsible-toggle links

https://gerrit.wikimedia.org/r/338045

Patch #7 was changing only the two "Collapsible table with caption" examples, which is fine and the expected behaviour.
Updated (#8) the patch, adding th .mw-collapsible-toggle to fix the "Collapsible table" example as well.

(as a reminder, on this example https://www.mediawiki.org/wiki/Manual:Collapsible_elements#Table a space is added between "Caption" and "[Collapse]". It isn't added in preview mode, nor on other wikis. Where does that come from?)
(update: it may be some issue in the Parser generating tables, which should be fixed, as it adds trailing spaces which are troublesome when selecting text. But that's really a separate issue.)

Od1n triaged this task as Medium priority.Nov 3 2017, 9:14 PM

@Krinkle Please see my the additional notes in my comment above.

Currently, the toggle is appended immediately next to the caption, so that there is no spaces. I pointed out above that in some setups the parser adds trailing spaces everywhere, but that's a different issue.

Another solution could be:

  • for table captions, insert a space before the toggler using javascript
  • for TH, add a margin as I'm currently doing

Still, for inline toggles in table captions, I would favor a margin over a space:

  • It's an UI widget, so that "natural spacing [of exactly one space]" doesn't really make sense. A bit more spacing wouldn't hurt, actually.
  • It doesn't create a space which would get selected with the rest of the text. (I hate so much these extraneous spaces in text selections!)

The changes should be quite robust as of now. I'd favor patch #9, it works on a specific "whitelist" set so it should be pretty safe, and more importantly, the space added by javascript in patch #10 really tickles me (see my comments above).

I assume T178998 is solving the exact same problem?

Yes, it's a duplicate. Though it got lucky and got more attention. Its CSS patch is less evolved, but note it has a workaround for the parser issue I mentioned earlier.

FYI, I have implemented my gerrit patch 9 on the French Wikipédia.

Change 338045 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/core@master] Add spacing before the mw-collapsible-toggle links

Reason:

As discussed.

https://gerrit.wikimedia.org/r/338045

By using margin-inline-start, I managed to considerably reduce the amount of CSS needed, see on French Wikipédia.

Also, currently (and contrarily to previously), in the HTML markup there is a space between the <caption> (or the first <th>) and the dynamically injected button, so there is now a spacing between them initially.

That's because the generated markup now looks like:

<caption>Text
</caption>
<th>Text
</th>

And the button gets appended after this newline, which results in a space between the elements.