1213791 - "suggested reviewers" menu overflows horizontally from visible area if reviewers have long name.

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: "suggested reviewers" menu overflows horizontally

Once fixed by bug 1140764, and regressed by updating jquery-contextmenu in bug 1183892.
upstream has overflow-detection for y-axis, but not for x-axis.

Maybe we should fix upstream?

Comment 1

[Tooru Fujisawa [:arai]]

Opened pull request for upstream
  https://github.com/swisnl/jQuery-contextMenu/pull/288

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Prevent suggested reviewers menu from overflowing from client area.

inserted |if(offset.left<0){offset.left=0}| after |if(offset.left+width>right){offset.left-=width}|.
same fix as PR for upstream.

Comment 3

[Tooru Fujisawa [:arai]]

FWIW, I checked the diff with http://jsbeautifier.org/

Comment 4

[:glob ✱]

Comment on attachment 8672563 [details] [diff] [review]
Prevent suggested reviewers menu from overflowing from client area.

Review of attachment 8672563 [details] [diff] [review]:
-----------------------------------------------------------------

let wait for your PR to be accepted upstream then update to a version that includes it.

Comment 5

[Tooru Fujisawa [:arai]]

I see :)
fortunately, this time the layout issue is not so annoying for most case.

Comment 6

[:glob ✱]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> fortunately, this time the layout issue is not so annoying for most case.

indeed; which is why i'm not in a rush to take it :)
jquery-contextMenu has a new maintainer, so i'd expect them to be much more responsive to PRs than the previous owner.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Screenshot with v1.10.1.

v1.10.1 has several changes both in JS and CSS, compared to current one.

following changes are notable:

https://github.com/swisnl/jQuery-contextMenu/commit/83e37fbd3c6b0ca113597c01f03f62757cf86ecb

this changes |.text(...)| back to |.html(...)|.  mail address disappears from the menu item, as it's parsed as HTML (will cause bug 1140798 again)


https://github.com/swisnl/jQuery-contextMenu/commit/461186990033a922477be6614f8d484cd0a229af

this adds |max-width: 250px;| to context menu.  It results in line wrap inside menu item, and possibly too long menu if there are many reviewers.


https://github.com/swisnl/jQuery-contextMenu/commit/f60094332a4e2993ad87fe152a714d9fbab3c7bf

this adds 24px padding-left to menu item.


For CSS changes, we could apply extra styles (or just update JS only?).  but for the JS change, we should escape labels in all places where it creates contextMenu :/

Comment 8

[Tooru Fujisawa [:arai]]

perhaps we should add 'plaintext' type that uses |.text(...)| to upstream?

Comment 9

[Tooru Fujisawa [:arai]]

I guess the JS change should be done in different way.  I'll post PR to upstream.

Comment 10

[Tooru Fujisawa [:arai]]

Opened another PR to change .html() back to .text() again
https://github.com/swisnl/jQuery-contextMenu/pull/299

Comment 11

[Tooru Fujisawa [:arai]]

https://github.com/swisnl/jQuery-contextMenu/pull/299 is also merged :)
I'll post a patch after next release.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Screenshot with v2.0.0.

Newer version changes its style again.
Here's screenshot with original style and modified style to widen the menu.
There still line wrap happens, because of following code that sets width of the menu, and it seems to be somehow wrong if we widen menu.
https://github.com/swisnl/jQuery-contextMenu/blob/c2af55c5ad2b1963e83af1f979420562317d2091/src/jquery.contextMenu.js#L1250-L1252

I'll try fixing it.  Still not sure what the best way here is tho :/

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1213791 - Part 1: Prevent suggested reviewers menu from overflowing from client area.

Updated contextMenu-min.js to v2.0.0.
Almost same as jquery.contextMenu.min.js in jQuery-contextMenu/dist but removed following line, as we're not using it.
> //# sourceMappingURL=jquery.contextMenu.min.js.map

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1213791 - Part 2: Update jQuery-contextMenu CSS to v2.0.0, with style fix.

Updated contextMenu.css to v2.0.0, with style fix to widen the menu with contextMenu-extra.css.
The content of contextMenu.css is copied from jQuery-contextMenu/dist/jquery.contextMenu.css, but removed @font-face and styles for icons (now they need font file)

contextMenu-extra.css does following
  * set maximum width to 100%, which was 360px
  * set horizontal padding to 8px, which was 24px
  * avoid wrapping line

Comment 15

[:glob ✱]

Comment on attachment 8680397 [details] [diff] [review]
Bug 1213791 - Part 2: Update jQuery-contextMenu CSS to v2.0.0, with style fix.

Review of attachment 8680397 [details] [diff] [review]:
-----------------------------------------------------------------

i'll let dylan review these, but i'm pretty sure we were carrying customisations to the context menu's css directly in contextMenu.css

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: "suggested reviewers" menu overflows too much horizontally

this becomes critical :P
https://bugzilla.mozilla.org/attachment.cgi?bugid=1289828&action=enter

I even cannot see who is there.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Update jQuery-contextMenu to v2.2.4.

Updated jQuery-contextMenu to v2.2.4, current latest release.
looks like with v2.2.4 we don't need to update CSS.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: comparison between current menu and v2.2.4

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8790998 [details] [diff] [review]
Update jQuery-contextMenu to v2.2.4.

r=dylan

Comment 21

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   7ab2e46..639a051  master -> master

Comment 22

[John A. Bilicki III]

...am I correct in my interpretation of this bug that someone implemented jQuery in to the Firefox code base?

Comment 23

[Tooru Fujisawa [:arai]]

(In reply to John A. Bilicki III from comment #22)
> ...am I correct in my interpretation of this bug that someone implemented
> jQuery in to the Firefox code base?

no, this bug is about bugzilla.mozilla.org code base.
nothing related to Firefox code base.