Non-breaking spaces break page layout when confirming thanks in diff view
Open, NormalPublic

Description

See the screenshot:

kaldari created this task.Sep 7 2015, 10:27 PM
kaldari updated the task description. (Show Details)
kaldari raised the priority of this task from to Needs Triage.
kaldari added a project: Thanks.
kaldari added a subscriber: kaldari.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptSep 7 2015, 10:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
kaldari set Security to None.

This seems to be due to jquery.confirmable in core rather than the Thanks extension.

kaldari renamed this task from Non-breaking spaces break page layout when using the Thanks feature on a diff to Non-breaking spaces break page layout when confirming thanks in diff view.Sep 7 2015, 10:35 PM
kaldari added a subscriber: matmarex.

@matmarex: Do you have any thoughts on how we could fix this without breaking it elsewhere? Maybe adding an extra parameter to $.confirmable() saying that the line should be breakable?

Catrope triaged this task as Normal priority.Sep 24 2015, 12:17 AM
Catrope added a subscriber: Catrope.

I don't think it's possible to animate this the way we do if we allow line-breaking (it relies on display: inline-block). Although it should be possible to allow line-breaking after the animation is done.

Well, I have managed to replicate the bug.
Here's the screenshot with display: inline-block: http://ctrlv.in/726549
And here's the screenshot without it: http://ctrlv.in/726550

The issue, indeed, seems to be with when display: inline-block is applied.

Now I am not too familiar with jquery, but know basic JS. Should I still go ahead with this task?

I'll take this up, and go ahead. :)

darthbhyrava added a comment.EditedMar 19 2016, 11:26 AM

Replicated this on my manual wiki installation - here's a paste.
The issue doesn't seem to be in core, rather in Thanks itself.

A proper combination of css styles for jquery.confirmable's wrapper, interface, text and mw-thank-link will probably do it.
So far, it's a bit skewed for me: another paste of progress so far.

Could someone tell me if I'm on the right track, please? @kaldari @Legoktm
Thanks. :)

jayvdb added a subscriber: jayvdb.Mar 24 2016, 1:04 AM

The issue doesn't seem to be in core, rather in Thanks itself.

I suspect that we wont know the answer to this question until you've found a solution which works with jquery.confirmable.
Have you looked for related bugs in jquery.confirmable ?

A proper combination of css styles for jquery.confirmable's wrapper, interface, text and mw-thank-link will probably do it.

Yes, this is an approach that should be explored. It would be great if you can find a tweak to the Thanks css and/or js which fixes this bug.

The issue doesn't seem to be in core, rather in Thanks itself.

I suspect that we wont know the answer to this question until you've found a solution which works with jquery.confirmable.
Have you looked for related bugs in jquery.confirmable ?

Yes, I have, in fact.
T89572: In Thanks confirmation, "No" choice title text should not say "Send a thank you notification to this user" seemed to be a good place to understand the jquery.confirmable code.

A proper combination of css styles for jquery.confirmable's wrapper, interface, text and mw-thank-link will probably do it.

Yes, this is an approach that should be explored. It would be great if you can find a tweak to the Thanks css and/or js which fixes this bug.

I'll go ahead with this, then! :)

I don't think it's possible to animate this the way we do if we allow line-breaking (it relies on display: inline-block). Although it should be possible to allow line-breaking after the animation is done.

After looking at it in more detail and talking to @jayvdb , I think this is probably how I should approach this problem. Hardcoding changes in my console worked in the browser, but there are many issues that come along with that apporach, and making changes to Thanks directly was probably the wrong way of looking at things.

Working on this now.

Change 283749 had a related patch set uploaded (by Darthbhyrava):
Non-breaking spaces break page layout when confirming thanks in diff view

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

Added a timeout function after the transitions which allows for line-breaking.

Feedback, please?

darthbhyrava added a comment.EditedMay 16 2016, 10:41 PM

In the wake of @matmarex's observation that error F3940024 crops up due to my patch, I looked into more details.

This is how the layout of the diff page works. The old edit is in the left column of a table inside a div, while the new edit is to the right.

<div id="mw-content-text">
    <table class=""diff diff-contentalign-left" data-mw="interface">
        <colgroup></colgroup>
        <body>
            <tr style="vertical-align: top;">
                <td class= "diff otitle"></td>
                <td class="diff ntitle">
                     {content}  
                </td>
            </tr>
        </body>
    </table>
</div>

Inside the right column "diff ntitle", there are further divs titled "mw-diff-ntitlei", where i ranges from 1 through 5.

The error arises because of the div "mw-diff-ntitle1", where the height seems to be garbled once the 'No' button is clicked.

Also, changing the word-wrap property of the mw-diff-ntitle1 div decreases the height.

However, I can't seem to figure out why this div would be affected by the patch I submitted which changes only white-line and width properties of the interface div post transition.

Pols12 added a subscriber: Pols12.Jun 17 2018, 3:24 PM

Besides, it would be nice if brackets would be kept with first and last words, in order to respect typographic rules (at least French typographic rules).

Hagarshilo added a comment.EditedJul 4 2018, 8:57 PM

Some things I've noticed that might help to fix this:

On .jquery-confirmable-interface, there is a

style="width: 0px;"

coming from somewhere. Before clicking cancel the value there is 346.312px, which looks like a calculation.
So I've searched MW for

style="width:

but did not find anything that seemed relevant. So I think this values is inserted during build by an automatic process.

I don't know how these processes work, so for now I just tried hardcoding the value so it doesn't change:

.jquery-confirmable-interface {
    width: 346.312px !important;
}

This way, clicking Cancel doesn't make the component drop to the bottom of the page, but the outcome is still not okay. The Cancel and Thank buttons are not removed.

When I change the value to something greater than 0, the text starts to get pulled back up.

At 200px it begins to looks almost normal (remember, it should be 346.312px). But we now have that 'thank' link there.

This screencap is from before clicking Cancel (i.e., before the action that changes the width value from 346.312 to 0):

Mooeypoo added a subscriber: Mooeypoo.EditedJul 5 2018, 3:31 PM

This is coming from the animation. Without your patch, there is an "opening" animation going on when the "Are you sure..." is appearing. In order to make it work, it has to start with width:0 and end with its natural width. When the string is not wrapping, the span "just" grows into the full line -- but when we make the string wrap, the changing width actually changes how much the text wraps -- which changes the height too.

I think there are two options here:

  • Cancel the wrapping ONLY for diff pages (leave history pages as-is)
  • Cancel the animation

Change 445426 had a related patch set uploaded (by Hagar Shilo; owner: Hagar Shilo):
[mediawiki/core@master] Disable animation on thanks confirmation to prevent page break

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

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 12 2018, 3:27 PM
JTannerWMF moved this task from Inbox to To Triage on the Growth-Team board.Jul 18 2018, 6:32 PM
NOTE: that T159302 is a related ticket that will change the wording of the Thank message. So that may change how this ticket is handled. T159302 is on the Growth team's active list for this quarter.
SBisson moved this task from To Triage to External on the Growth-Team board.Jul 20 2018, 6:15 PM