New issue
Advanced search Search tips

Issue 722217 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocked on:
issue 791391



Sign in to add a comment

Text in "alert" boxes not selectable

Reported by sie...@boerse-go.de, May 15 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce the problem:
1.  alert('some Text')
2.  try to select "some Text" in the dialog box
3.  => not working

What is the expected behavior?
Text should be selectable

What went wrong?
Text is NOT selectable

Did this work before? N/A 

Chrome version: 58.0.3029.110  Channel: stable
OS Version: OS X 10.12.4
Flash Version: 

probably a regression:
https://bugs.chromium.org/p/chromium/issues/detail?id=5879
but this ticket is closed according to last comment
 
Labels: Needs-Triage-M58 Needs-Bisect
Cc: jmukthavaram@chromium.org
Labels: -Needs-Bisect M-60
Status: Untriaged (was: Unconfirmed)
Observations:
------------
1.Able to reproduce the issue on mac 10.12.4 with Fresh chrome (58.0.3029.110) installation ( Uninstall existing chrome & install new chrome build)
2.Unable to reproduce the issue on mac 10.12.4 using chrome reported version-58.0.3029.110 when chrome is over installed on old version. 
3.Issue working as intended from M30 builds to latest stable only when user over installed chrome.

Note:
Unable to reproduce the issue on Windows 7 & ubuntu 14.04 using chrome reported version-58.0.3029.110.

Removing needs-bisect label & marking it as 'untriaged' to get more inputs from dev.
Thank you..!!
 Issue 719978  has been merged into this issue.
Components: -UI UI>Browser
Owner: ellyjo...@chromium.org
Status: Assigned (was: Untriaged)
[chrome mac triage] Assigning this ellyjones@ since she has been working on dialogs.
Cc: tapted@chromium.org
Labels: -M-60 M-63
Status: Started (was: Assigned)
I had a look at fixing this for MacViews. It seems that, while the label is configured to be selectable, it never actually receives OnMouseDown/OnMouseDragged (?). I tried in views_examples_with_content_exe as well, and it appears that Views labels do not support text selection. I remember karandeepb@ working on this as part of some earlier MacViews work but I'm not sure the state of it. tapted@, do you remember what was going on with that?

For Cocoa, the fix is not too hard: <https://chromium-review.googlesource.com/c/chromium/src/+/714036>
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9d5e38c448579d1b61ce10fd9dd204ddecb6148

commit e9d5e38c448579d1b61ce10fd9dd204ddecb6148
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Wed Oct 11 21:05:01 2017

cocoa: make javascript dialog text selectable

This change:
1) Makes javascript dialog title and message selectable;
2) Moves the attributes of the title and message onto the NSTextField{,Cell} as
   appropriate instead of the NSAttributedString, because otherwise selecting
   the text reverts the string attributes (!).

Bug:  722217 
Change-Id: I281187e2b1c14e80d0b346401b7caf41b1cc4326
Reviewed-on: https://chromium-review.googlesource.com/714036
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508112}
[modify] https://crrev.com/e9d5e38c448579d1b61ce10fd9dd204ddecb6148/chrome/browser/ui/cocoa/constrained_window/constrained_window_alert.mm

Cc: ellyjo...@chromium.org
Owner: tapted@chromium.org
The Cocoa side of this is fixed. I'm kicking the Views side over to tapted@.

Comment 9 by tapted@chromium.org, Oct 11 2017

We need to use RenderTextHarfbuzz in the Label.

changing

void Label::Init(const base::string16& text, const gfx::FontList& font_list) {
  render_text_.reset(gfx::RenderText::CreateInstance());

to

void Label::Init(const base::string16& text, const gfx::FontList& font_list) {
  render_text_.reset(gfx::RenderText::CreateInstanceForEditing());

should fix this.. We may even be able to do that in SetSelectable..
Labels: TE-Verified-M63 TE-Verified-63.0.3238.0
Verified the fix on Mac 10.12.6 using latest chrome version #63.0.3238.0 as per the comment #0.
Attaching screen cast for reference.
Observed that "some Text" in the dialog box got selected as expected.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
722217.mp4
523 KB View Download
Owner: ellyjo...@chromium.org
Got it - thanks tapted@ :) I'll have a go at implementing this.
Ooh, note we are _probably_ not quite ready to go all-in on 

Label::Init(const base::string16& text, const gfx::FontList& font_list) {
  render_text_.reset(gfx::RenderText::CreateInstanceForEditing());

There are some emoji I want to fix.

but something like that in SetSelectable(), guarded by #if MAC, and followed by a call to Label::CreateRenderText() or similar, is probably OK.
tapted: what's the situation with the emoji? and/or is there a bug for that?

I'd like to make the Views side of this fix if we can.
Emoji were hopefully fixed in  Issue 739641 . This should be good to go forward. There are performance concerns -  Issue 785522  - but they should be fixed for m65, and probably are not noticeable when there are fewer than ~100 lines of text.
Elly: heads up - I'm planning just to make views::Label always use RenderTextHarfBuzz on Mac. Multiline labels are currently super-slow because RenderTextMac does not support multiline, so views::Label can not cache its size calculations.

See  Issue 789118 . RTM is about 10x slower than RTHB (100ms vs 10ms). I explored a simple caching fix for RTM, but I don't think it's worth the effort.
Cool - is there a CL up for that?
Blockedon: 791391
> Cool - is there a CL up for that?

https://chromium-review.googlesource.com/c/chromium/src/+/805534

('ware dragons)
That CL landed as r522617, but it may have stirred up a spiders nest. Hopefully it will stick.

In other news, that change also simply causes this issue to be fixed \o/.
Cc: -tapted@chromium.org
Owner: tapted@chromium.org
In that case, I'm assigning this to you :)
Status: Fixed (was: Started)
That CL landed. This is fixed with chrome://flags/#secondary-ui-md enabled , target: ~m66.


Screen Shot 2018-01-30 at 3.49.27 pm.png
17.6 KB View Download

Sign in to add a comment