Opened 4 years ago

Last modified 3 years ago

#13612 review Bug

[mathjax] long formula causes dialog window to go out of the viewport

Reported by: Wiktor Walc Owned by: kkrzton
Priority: Nice to have (we want to work on it) Milestone:
Component: UI : Dialogs Version: 4.3
Keywords: Cc:

Description (last modified by Wiktor Walc)

Found on Chrome.

  1. Make sure the browser width is like 1000px or so.
  2. Enter the following formula in the mathjax dialog:
    x = {-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}{-b \pm \sqrt{b^2-4ac} \over 2a}
    
  3. See that the Ok/Cancel buttons are unavailable.

Attachments (1)

Screen Shot 2015-07-31 at 13.15.53.png (72.0 KB) - added by Wiktor Walc 4 years ago.

Download all attachments as: .zip

Change History (21)

Changed 4 years ago by Wiktor Walc

comment:1 Changed 4 years ago by Wiktor Walc

Description: modified (diff)

comment:2 Changed 4 years ago by Wiktor Walc

Description: modified (diff)

comment:3 Changed 4 years ago by Wiktor Walc

Status: newconfirmed

comment:4 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7

comment:5 Changed 4 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:6 Changed 4 years ago by kkrzton

Status: assignedreview

Changes in t/13612

This fix adds relayouting of the dialog whenever mathajx preview is rendered. It means dialog always will be centered unless user moved it manually. When dialog was moved manually, its' coordinates are not overwritten so that the user does not lose controll over positioning the dialog.

comment:7 Changed 4 years ago by kkrzton

I think also #14262 should be taken into consideration.

comment:8 Changed 4 years ago by Marek Lewandowski

Status: reviewreview_failed
  1. It's a dialog only issue, so it's a dialog responisbility to provide a fix/proper logic for that.

Proposed fix is within the general use function, which is unnecessary. It could be simply added to the dialog's proper field onLoad, like keyup listener here.

BUT here another fix would be applicable... :) we should not only listen for keyboard events, but any change to the texarea, e.g. paste event (using mouse) so keyup is not necessarily the best event here.

Please fill a new issue for MathJax dialog not updating on mouse paste and reference this ticket. Then go back to current ticket and the change existing event listener - once it gets R+ it will fixed related ticket as well.

  1. Could I also ask you for a minor refactorization here? It would be sweet to extract !( CKEDITOR.env.ie && CKEDITOR.env.version == 8 ) to variable like isSupportedBrowser, it'd improve readability.
Last edited 4 years ago by Marek Lewandowski (previous) (diff)

comment:9 Changed 4 years ago by kkrzton

Status: review_failedassigned

comment:10 Changed 4 years ago by kkrzton

Ad. 2

I agree it is a dialog only issue but listening on input is not enough. There is a case when preview can change without any prior events trigger. It happens when already inserted formula is double clicked in editor. The dialog is opened and textarea updates with focused equation. No input events are trigerred. So relayouting should also happen when values are changed programmatically.

The second thing is that we assume this is a dialog only issue and dialog is responsible for its size. The size is really controlled by content of the dialog, in this case mathjax iframe. It indirectly sets dialog width by setting preview iframe size.

So to be consistent we should assume one:

  • content can control dialog size (without any further steps - it works now this way)
  • content can control dialog size but must inform dialog about changes in its size (so e.g. the dialog can relayout)
  • content can ask dialog to resize (but is's up to dialog to decide if it can be resized)
  • dialog watches for changes in content and manages its size

My favorite is 2 because it is flexible and partially same as current behavior but gives a dialog additional possibilities to react to changes (and here i assume mathjax content can inform dialog after resizing).

It looks somehow complicated as for such simple issue but it's worth considering how it should work.

comment:11 Changed 4 years ago by kkrzton

The case with editing formula mentioned above could be also solved with simple layout() call after updating dialog content. It is little less consistent but much simpler so I'll probably go in this direction:)

comment:12 Changed 4 years ago by kkrzton

So I found the exact case which bothered me and why relayouting dialog after mathajx update event makes sense (also previously written test case fails because of that).

For the solution proposed in comment:8 the case looks like this:

Normally mathjax rendering is synchronous so calling dialog.layout() in input listener after setting new value (which also means rerendering) works and executes in proper order: rerender -> relayout. The problem is when mathjax dialog is opened for the first time. Mathjax lib/iframe is loaded in asynchronous manner. In this case when the above long formula is pasted and mathjax iframe is not already loaded, layouting happens before rendering preview (preview waits for mathjax to complete loading).

This case can be observed easily (with proposed fixes ofc) using throttling option in Chrome Devtools (network tab). For my localhost test it occurs up to DSL(2Mb/s 5ms RTT). To reproduce:

  1. Open mathjax dialog
  2. As soon as it opens paste long formula

It is really the edge case. If we would like to solve this I will go with comment:10 as a solution if not comment:8 solution is sufficient.

comment:13 Changed 4 years ago by kkrzton

Status: assignedreview

Changes in t/13612.

Introduced isSupportedBrowser variable as suggested.

Added callback for mathjax preview update so the function can be passed from dialog when creating preview.

Also fixed #14299.

comment:14 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:15 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:16 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:17 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:18 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:19 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:20 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy