Opened 7 years ago

Closed 7 years ago

#9248 closed Bug (fixed)

SCAYT's context menu option "more suggestions" doesn't fit

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.0
Component: UI : Context Menu Version: 4.0
Keywords: Cc:

Description

  1. Enable SCAYT.
  2. Right click on misspelled word.
  3. "More suggestions" is hidden under the arrow on the right.

Attachments (2)

Workspace 1_002.png (11.0 KB) - added by Garry Yao 7 years ago.
9248.html (1.1 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by Garry Yao

Attachment: Workspace 1_002.png added

comment:1 Changed 7 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: newassigned

comment:2 Changed 7 years ago by Olek Nowodziński

Status: assignedreview

Created branch t/9248 with the solution.

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Unfortunately the proposed solution (read it as ugly hack) is not acceptable. We need instead to be focused on fixing it by proposing DOM+CSS changes.

comment:4 Changed 7 years ago by Olek Nowodziński

Alright, but DOM changes might result in totally new context menu since I've already tried to gently fix it with CSS and I failed. The problem is that the current context menu implementation is very limited and it's hard to change something but not to change the entire template which is, of course, something undesirable.

comment:5 Changed 7 years ago by Olek Nowodziński

Status: review_failedreview

Created branch t/9248c with another, DOM-oriented approach. This time it's not so dirty. I'd rather say it's tricky. Please consider all the browsers and HC mode when reviewing to avoid regressions.

comment:6 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

After the patch, arrow is not showing in long label text in RTL on the first open, check the attached sample to reproduce.

Changed 7 years ago by Garry Yao

Attachment: 9248.html added

comment:7 Changed 7 years ago by Garry Yao

I've pushed t/9248b, another approach that use positioning, it's less tricky and simpler, the only exception is that it will not work well for RTL in IE7 because of a current regression.

comment:8 in reply to:  7 Changed 7 years ago by Olek Nowodziński

Status: review_failedreview

Replying to garry.yao:

I've pushed t/9248b, another approach that use positioning, it's less tricky and simpler, the only exception is that it will not work well for RTL in IE7 because of a current regression.

Thank you for the contribution, Garry! However I finally reached desired look among all the browsers except Opera, which is broken much more than this. I rebased t/9248c on t/9355 (#9355) with a small fix and it's fine now.

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Ok, the result looks good for t/9248c, which is a good rescue for IE7, while it still breaks Opera & IE quirks, which looks fine for master at least.

The think is that we must keep in mind to have a clean main CSS, for good browsers, the quirks that are affecting bad browsers should be dedicated to their own file, read as the reason why we create the browser skin part feature, and to accelerate skin developing (not forcing them to consider browser compatibility at the first time), so in this case, float is only required for IE7 (to stretch properly the menu in RTL).

I've ported the float solution proposed by t/9248c, on top of t/9248b, for IE7, I'd ask to switch the review on t/9248c at this point.

I've R+ and landed #9355 to accelerate the review.

comment:10 Changed 7 years ago by Olek Nowodziński

Status: review_failedreview

Thanks for #9355! t/9248 looks good. This is something we can use if other methods fail.

Anyway, I discovered a completely different approach which is based on display: table-* and I pushed this to t/9248d. No floating, it's intuitive, and the only hacks are for IE7.

  • Works everywhere in LTR and RTL (IE7+, FF3.6, FF latest, Chrome).
  • The only exception is Opera. It fails in RTL. I think it's either Opera bug or something's wrong with these lines. It'd be great if you, Garry, tried to figure this out as it's pretty weird for me because this display:table-* works fine in Opera while LTR is set.
  • t/9248d is a pre-review thing as HC hasn't been implemented yet to save time.
  • Check this out ;)

I hope we'll handle this Opera RTL thing, finish HC, and this solution will be final.

PS: We could eventually use <table> markup as a solution as well, which might be more bulletproof than display:table-*.

comment:11 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Brilliant approach, table is very suitable CSS presentation of this layout requirement as well, but it imposed an extra "cke_menubutton_wrap" just for putting a "display:table" on it, which I can tell is just not required, "row" and "cell" is enough to make it right.

comment:12 Changed 7 years ago by Olek Nowodziński

Status: review_failedreview

Updated t/9248d:

  • Removed cke_menubutton_wrap. It was unnecessary, thanks for pointing this out!
  • Fixed HC mode.

Please, test all the possible configurations (browser+RTL+HC) as this is a crucial part of UI.

The only bug I see is a weird RTL in Opera.

PS 1. Do we care about quirks? PS 2. If R+, I'll squash it a little bit.

comment:13 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

Good to go with t/9248d.

comment:14 Changed 7 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Fixed by the commit.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy