Page MenuHomePhabricator

Page becomes horizontally scrollable with certain formulae
Closed, ResolvedPublic

Description

Scroll down to fundamental forces heading on:
https://en.m.wikipedia.org/wiki/Standard_Model?oldid=707260009

The app has some logic which imposes a max-width of 100% to all outputted formulae from the Math extension.

Right now it has to analyse the class name to do this since there is no generic class name.

I recommend we

  • add a generic class mwe-math-element to all math formulae. This will allow the app to style this via a single css rule
  • Add a max-width to the element in the Math extension so mobile web can benefit.

Test Plan

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Mholloway Any reason this task is assigned to you?

@Nirzar At grooming, it was determined that one of @Jdlrobson 's solutions above (https://phabricator.wikimedia.org/T134281#2282141) would need to be chosen by you, and a followup task created specifically around that. Please do so? :)

Mholloway removed Mholloway as the assignee of this task.Sep 29 2016, 8:29 PM
Mholloway added a subscriber: Mholloway.

Was going to work on it but spaced, sorry. Someone else is welcome to have at it.

Set max-width to 100% and avoid horizontal scrolling altogether - a user can switch to landscape mode to get a better look.

this seems more optimal and a way to go

The image could be wrapped in a container and use overflow on that container,

This could be nice if we have a visual cue to indicate there is internal scrolling. there is a pending task for that if i remember correctly. but it will go into a bigger loop for this solution.

TameeshB added a subscriber: TameeshB.EditedNov 2 2016, 5:11 PM

May I please take up this bug for my first bug fix?

Jdlrobson assigned this task to TameeshB.Nov 2 2016, 5:20 PM

Yes! good luck! :)

As in your previous comment,
Which solution must I go with?

  1. Set max-width to 100% and avoid horizontal scrolling altogether - a user can switch to landscape mode to get a better look.

The above one might cause problem in a case that the equation is a long one, it would become near invisible.

  1. The image could be wrapped in a container and use overflow on that container,

So should I go with this^ ?

@TameeshB sounds good to me! @Physikerwelt let us know if you see a problem with that asap :)

I found that all the math equations are enclosed in a "<dd>" which itself is wrapped by a "<dl>" tag.
Is it okay to add the css properties to the selector

dl>dd{

}

?

@TameeshB I'm not too familiar with the extension but if they are all wrapped in a dl tag, it sounds like that would make sense. If you're confident, submit a patch and we'll explore that in a code review.

Change 320978 had a related patch set uploaded (by TameeshB):
Fix Bug T134281 Math-page-overflow

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

I'm not sure what to say.


Obviously, I'm disappointed about the lazy image loading. It causes that the math fallback images are shown either way.
For the scrolling problem (this bug) I see no way to improve the situation.
I think in those rare cases where the formula is longer than a line a scroll bar is fine.
Making the formulae smaller does not seem a good alternative to me.
Adding additional elements might cause problems in other cases.
Thus I suggest to leave the situation and focus on fixing the more obvious problems like duplicated math images first.

@Physikerwelt I think the current idea is just to add a class ( not alter the markup anymore) and allow horizontal scrolling (don't change size). @TameeshB did you have any luck locating the code outputting the dd element?

I believe the duplicate math image was due to the [[ T144445 | Firefox browser extension ]]? Or are you seeing this elsewhere? I'm hoping we have bandwidth in December to look at this and other related Math/lazy image problems. I'm sorry we have been unable to find the time so far, we have too many extensions and too few people.

ovasileva raised the priority of this task from Normal to High.Nov 21 2016, 7:40 PM

w.r.t dd:

People use two different options to set displaystyle formulae. <math display=block> ... or :<math>. We could alter the css of .mwe-math-fallback-image-display to change the scrolling behavior. If block is used the fallback image will be put into a div-element
https://github.com/wikimedia/mediawiki-extensions-Math/blob/238dd240e4a507f0a1a66f099908fc5afae546a4/MathMathML.php#L445

I have no idea how to handle :<math> other than using a bot to replace :<math by <math display=block.

@TameeshB did you have any luck locating the code outputting the dd element?

I'll resume working on it this Friday as I have exams until then.

@Jdlrobson @Physikerwelt
I've been trying to wrap the "mwe-math-fallback-image-inline" inside a div(i've tried it in chrome developer console and it works).
But to wrap it up, I tred searching for the <img...> markup using grep and there was no proper HTML markup in any php file, all of them were filled up with definitions of classes, except for this one file "texvc_cgi.ml"
line 31 on extensions/Math/texvc_cgi.ml

   out ("<h3>Image:</h3><img src=\""^finalurl^md5^".png\">")
	with Util.FileAlreadyExists -> out ("<h3>Image:</h3><img src=\""^finalurl^md5^".png\">")

Will wrapping this img element work? Or is there some other php responsible for the output.
The only other relevant php i could find was in
line 147 /extensions/Math/MathTexvc.php (searching for class name in grep)

if ( $this->getMathStyle() === 'display' ){
			// if DisplayStyle is true, the equation will be centered in a new line
			$attributes[ 'class' ] = 'mwe-math-fallback-image-display tex';
		}

But if the above is responsible for the output, it isn't a regular HTML markup, how do I wrap it in the div in this case then?

I'm not sure if wrapping inline math elements in a div would be a good idea. And texvc_cgi is not used in production. The code is actually here
https://github.com/wikimedia/mediawiki-extensions-Math/blob/master/MathMathML.php#L453

I'm not sure if wrapping inline math elements in a div would be a good idea. And texvc_cgi is not used in production. The code is actually here
https://github.com/wikimedia/mediawiki-extensions-Math/blob/master/MathMathML.php#L453

Wow, thanks for the code reference! :D

TameeshB added a comment.EditedDec 5 2016, 6:31 PM

@Jdlrobson, I've written the patch but am not sure as I'm not able to test it as every time I try to save a math page, I receive a database error

A database query error has occurred. This may indicate a bug in the software.[WEWyJ38AAQEAACM@QwoAAAAF] 2016-12-05 18:29:59: Fatal exception of type "DBQueryError"

Ive commented my changes and checked, the error still shows up.
What Should I do? Do I Submit the patch?

in the mediawiki core folder try running

maintenance/update.php

Hopefully this will fix that problem. Otherwise you'll need to debug this. Do you have the problem when you use your master branch or is this a problem caused by your patch?

@Jdlrobson I ran update.php, installed php-mysql module It showed the same error.
I deleted the extension and the downloaded a fresh stable version and run update.php and its still showing the same error.

@TameeshB can you add these to your LocalSettings.php and refresh the page. You should get more info:

error_reporting( -1 );
ini_set( 'display_errors', 1 );
$wgShowSQLErrors = true;
$wgDebugDumpSql  = true;

@bmansurov I tried later on Windows, it worked.
@Jdlrobson @Physikerwelt What parsing options should I choose in

$wgDefaultUserOptions['math'] = '?';
$wgMathFullRestbaseURL= '?';
$wgMathValidModes=array( 'png', 'source', 'mathml' );

For

$wgDefaultUserOptions['math'] = 'mathml';
$wgMathFullRestbaseURL= 'https://api.formulasearchengine.com/';
$wgMathValidModes=array( 'png', 'source', 'mathml' );

I'm getting error as:

Sounds like a problem with your setup connecting to Restbase (e.g. wrong url). Are you using mediawiki vagrant?
The documentation on http://api.formulasearchengine.com/v1/?doc may provide some clues if not.

the config looks good to me. Can you access https://api.formulasearchengine.com from your machine?

@TameeshB need any help with the task?

@Physikerwelt Yes, i'm able to access the url form my browser.
@bmansurov Yes, I had written the patch but i wasn't able to test it.

@TameeshB that's great. Please upload the patch at your convenience. We maybe able to help out with testing it.

Okay cool, i'll upload it! @bmansurov

Change 326996 had a related patch set uploaded (by TameeshB):
Issue T134281'

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

Change 326996 had a related patch set uploaded (by TameeshB):
Add class mwe-math-element to math elements

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

TameeshB closed this task as Resolved.Dec 21 2016, 10:53 PM
TameeshB reopened this task as Open.Dec 21 2016, 10:55 PM
bmansurov updated the task description. (Show Details)

Change 326996 merged by jenkins-bot:
Add class mwe-math-element to math elements

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

bmansurov updated the task description. (Show Details)Dec 21 2016, 11:28 PM

@bmansurov please do not self merge your changes.

In general, I think it's a good idea to add an element name to the outermost HTML element generated by the Math extension. However, I have the feeling that it would be better to use a fixed element name. For example the name in css file is static. Moreover, we do not use globals to access the mw* settings in the math extension.

I just tested on beta, but I could not see an effect from the change.


(It's interesting that the double math tags disapear in preview mode)


Inline math looks ok. (Even though the MathML baseline looks much better compared to the SVG baseline)

In general, I think it's a good idea to add an element name to the outermost HTML element generated by the Math extension. However, I have the feeling that it would be better to use a fixed element name. For example the name in css file is static.

Can you give an example of a fixed element name?

Moreover, we do not use globals to access the mw* settings in the math extension.

There are already instances of globals in the codebase, e.g. $wgMathCheckFiles, $wgMathDefaultLaTeXMLSetting, etc. (I have not checked others). I'm not saying it's good, but since it's an established practice in the extension, I thought it would be OK to use globals here too.

As for the changes, I see that the CSS rule is not appearing in the front end. I'm not sure why yet.

Change 328694 had a related patch set uploaded (by Bmansurov):
Fix the math element class name

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

@bmansurov I dont think applying the css to the span and the img tags would work as i've checked doing that in the chrome editing the markup by "inspect element".
It worked when I applied the css on the wrapping <dd> element.


^ this worked

Hmm, that's weird. It works for me.

Can you give an example of a fixed element name?

mwe-math-fallback-source-display
it is used in the css and javascript code

There are already instances of globals in the codebase, e.g. $wgMathCheckFiles, $wgMathDefaultLaTeXMLSetting, etc. (I have not checked others). I'm not saying it's good, but since it's an established practice in the extension, I thought it would be OK to use globals here too.

Sorry, I messed that up. I was referring to https://www.mediawiki.org/wiki/Manual:Configuration_for_developers but it seems this was not yet applied to the Math extension.

Testing on iPhone 6S (iOS 10.1.1), this is not fixed yet as on three apps I've observed the math images still trail off the page:

Safari

Firefox

Chrome

TheDJ added a subscriber: TheDJ.EditedJan 2 2017, 9:13 PM

@Nicholas.tsg resolved is not the same as deployed and live for all users.... That cycle starts the 3rd of january, as mentioned in the deploy tag. See also https://www.mediawiki.org/wiki/Bug_management/Bug_report_life_cycle#When_can_I_use_the_fix

There is a revert @ Iea5f231e0cea1221dc717b3409b36c0baf11bb24 - I've not merged it as the commit message needs updating but wants that's done we should merge.

there seems to be two problems with the patch that was merged.

  1. The css rule points to the wrong thing.
  2. The class applies to the image rather than a container around the image meaning the image is never scrollable and always the width of the screen.

Change 328694 abandoned by Bmansurov:
Fix the math element class name

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

Jdlrobson removed a project: Patch-For-Review.

@TameeshB would you like to have a second attempt at this?

bmansurov removed Nicholas.tsg as the assignee of this task.Jan 4 2017, 6:10 PM
bmansurov added a subscriber: Nicholas.tsg.
Physikerwelt lowered the priority of this task from High to Normal.Jan 11 2017, 6:00 PM
Physikerwelt moved this task from Incoming to Doing on the Math board.

In the following patch, I'll present a solution for displaystyle (<math display=block>...) formulae (not for inline formulae)

, please do not expect it to work on inline formulae.

Change 331661 had a related patch set uploaded (by Physikerwelt):
Add scrollbars to displaystyle formulae

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

Physikerwelt moved this task from Doing to Incoming on the Math board.Jan 11 2017, 7:06 PM
Physikerwelt added a comment.EditedJan 16 2017, 8:31 AM

@Jdlrobson @bmansurov Could you please have a look at that 6 line change. I think that's a first step. For the rest, of the elements I recommend the following

  • inline math elements (such as For <math>x\in \reals</math> it follows that <math>x^2 \ge 0. </math>)
    • -> If they are too long they should probably be reformatted as displaystyle equations. While it's usual to break long words into several lines, that's not usual for mathematical expressions
  • math elements formatted as :<math (such as :<math> \mathcal{L}_{\mathrm{Fermion}}(\phi, A, \psi) = \overline{\psi} \gamma^{\mu} D_{\mu} \psi + G_{\psi} \overline{\psi} \phi \psi,</math> from https://en.wikipedia.org/wiki/Higgs_mechanism)
    • -> either write a bot to convert them to <math display=block elements so that the ExtensionMath knows that this are displaystyle math tags (preferred) or
    • -> change to formatting of : in core (can not be in the math extension and would also effect other long, unbreakable elements formatted this way. However, this seems to be a different task).

Sorry for the delay in review! I have been on vacation. I will review this further on the beta cluster when it gets deployed there as my local examples are limited.

@Jdlrobson thank you. You can check https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:Version to see the latest commit on beta or investigate the process here https://integration.wikimedia.org/zuul/ it will take round about 15 min. I changed one fromulae on beta to demonstrate the effect.

Change 331661 merged by jenkins-bot:
Add scrollbars to displaystyle formulae

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


mh ... while the block is scrollable the MathJax extension for chrome does not recognize it

This looks to be behaving like it should be to me? That second block is not display block I see... so would not be fixed by your change. Am I missing something? (Testing without MathJax)

Physikerwelt closed this task as Resolved.Jan 18 2017, 9:05 AM
Physikerwelt claimed this task.

@Jdlrobson @bmansurov Could you please have a look at that 6 line change. I think that's a first step. For the rest, of the elements I recommend the following

  • inline math elements (such as For <math>x\in \reals</math> it follows that <math>x^2 \ge 0. </math>)
    • -> If they are too long they should probably be reformatted as displaystyle equations. While it's usual to break long words into several lines, that's not usual for mathematical expressions
  • math elements formatted as :<math (such as :<math> \mathcal{L}_{\mathrm{Fermion}}(\phi, A, \psi) = \overline{\psi} \gamma^{\mu} D_{\mu} \psi + G_{\psi} \overline{\psi} \phi \psi,</math> from https://en.wikipedia.org/wiki/Higgs_mechanism)
    • -> either write a bot to convert them to <math display=block elements so that the ExtensionMath knows that this are displaystyle math tags (preferred) or
    • -> change to formatting of : in core (can not be in the math extension and would also effect other long, unbreakable elements formatted this way. However, this seems to be a different task).

That the second thing that looks like a block was not changed was expected, see above. The problem with MathJax and also with MathML is a different story.