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

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

Production

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8672556 [details]
"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?
(Assignee)

Comment 1

3 years ago
Opened pull request for upstream
  https://github.com/swisnl/jQuery-contextMenu/pull/288
(Assignee)

Comment 2

3 years ago
Created attachment 8672563 [details] [diff] [review]
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.
Assignee: nobody → arai.unmht
Attachment #8672563 - Flags: review?(dkl)
(Assignee)

Comment 3

3 years ago
FWIW, I checked the diff with http://jsbeautifier.org/
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.
Attachment #8672563 - Flags: review?(dkl) → review-
(Assignee)

Comment 5

3 years ago
I see :)
fortunately, this time the layout issue is not so annoying for most case.
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8678604 [details]
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 hidden (obsolete)
(Assignee)

Comment 9

3 years ago
I guess the JS change should be done in different way.  I'll post PR to upstream.
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

3 years ago
Created attachment 8680340 [details]
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 :/
(Assignee)

Comment 13

3 years ago
Created attachment 8680395 [details] [diff] [review]
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
Attachment #8672563 - Attachment is obsolete: true
Attachment #8680395 - Flags: review?(glob)
(Assignee)

Comment 14

3 years ago
Created attachment 8680397 [details] [diff] [review]
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
Attachment #8680397 - Flags: review?(glob)
Attachment #8680395 - Flags: review?(glob) → review?(dylan)
Attachment #8680397 - Flags: review?(glob) → review?(dylan)
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
(Assignee)

Comment 16

2 years ago
Created attachment 8775417 [details]
"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.
(Assignee)

Comment 17

2 years ago
Created attachment 8790998 [details] [diff] [review]
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.
Attachment #8672556 - Attachment is obsolete: true
Attachment #8678604 - Attachment is obsolete: true
Attachment #8680340 - Attachment is obsolete: true
Attachment #8680395 - Attachment is obsolete: true
Attachment #8680397 - Attachment is obsolete: true
Attachment #8775417 - Attachment is obsolete: true
Attachment #8680395 - Flags: review?(dylan)
Attachment #8680397 - Flags: review?(dylan)
Attachment #8790998 - Flags: review?(dylan)
(Assignee)

Comment 18

2 years ago
Created attachment 8790999 [details]
comparison between current menu and v2.2.4
Comment on attachment 8790998 [details] [diff] [review]
Update jQuery-contextMenu to v2.2.4.

r=dylan
Attachment #8790998 - Flags: review?(dylan) → review+
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
To git@github.com:mozilla-bteam/bmo.git
   7ab2e46..639a051  master -> master
(Assignee)

Updated

2 years ago
Depends on: 1305647
...am I correct in my interpretation of this bug that someone implemented jQuery in to the Firefox code base?
(Assignee)

Comment 23

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.