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

Od1n created this task.Jan 15 2017, 2:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 15 2017, 2:45 PM
Od1n added a comment.Jan 15 2017, 2:54 PM

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');
Prtksxna updated the task description. (Show Details)Jan 16 2017, 5:44 AM
Prtksxna updated the task description. (Show Details)Jan 16 2017, 7:47 AM

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

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


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
|}

Od1n added a comment.Jan 16 2017, 9:03 AM
  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.
Prtksxna added a comment.EditedJan 18 2017, 9:15 AM

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.

Fito awarded a token.Feb 2 2017, 6:46 AM
Fito added a subscriber: Fito.

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

Od1n added a comment.Feb 16 2017, 3:51 AM

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

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

Prtksxna assigned this task to Od1n.Jun 5 2017, 2:12 AM
Od1n added a comment.Jun 5 2017, 6:28 AM

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.)

@Krinkle any update on this?

Od1n removed Od1n as the assignee of this task.Jul 13 2017, 3:31 AM
Od1n added a comment.EditedNov 1 2017, 6:31 PM

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.

Od1n added a comment.Nov 1 2017, 6:51 PM

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.

Od1n added a comment.Nov 1 2017, 7:07 PM

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

Od1n added a comment.EditedNov 2 2017, 5:25 PM

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 Normal priority.Nov 3 2017, 9:14 PM
Od1n added a comment.Nov 4 2017, 1:12 AM

@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
Od1n added a comment.EditedNov 4 2017, 11:56 PM

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!)
matmarex removed a subscriber: matmarex.Nov 7 2017, 11:43 PM
Od1n added a comment.Nov 14 2017, 6:35 PM

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?

Od1n added a comment.Dec 5 2017, 1:40 PM

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.