New issue
Advanced search Search tips

Issue 648560 link

Starred by 1 user
Status: Archived
Owner:
lgrey@chromium.org
Closed: Sep 19
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

cocoa browser: bookmarks bar is not RTL-aware

Project Member Reported by ellyjo...@chromium.org, Sep 20 2016 
The bookmarks bar is laid out LTR even when the browser is in RTL mode.

Comment 1 by lgrey@chromium.org, Nov 15 2016

Owner: lgrey@chromium.org
Status: Started (was: Available)

Comment 2 by avi@google.com, Feb 18 2017 

I've been playing with this, and it's insane. I'm getting different results in the first window that I open vs subsequent ones; I'm having trouble getting obvious code fixes to work, etc. Sigh.

Here's my current list of things to fix/test:
- test button layout not full
- test button layout full, ensure overflow chevron button correctly shows
- test "gimme bookmarks" with empty
- test dragging side to side
- menus should align to right edge of parent item
- menus should have right-aligned items
- icons should be on the right of labels
- overflow arrow for submenus should be pointing in the correct direction
- overflow chevron should be pointing in the correct direction
- test bookmark buttons with LTR text
- test bookmark buttons with RTL text
- correct binding on a window resize
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 8 2017 

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

commit 67b81bc38c7e2164bc77f0c00d11d26d620edc18
Author: lgrey <lgrey@chromium.org>
Date: Wed Mar 08 16:39:35 2017

[Mac] Remove bookmark bar NIB
BUG= 648560 

Review-Url: https://codereview.chromium.org/2729603008
Cr-Commit-Position: refs/heads/master@{#455468}

[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/app/nibs/BUILD.gn
[delete] https://crrev.com/02ebf8339c19e2972d49d5fbbade603be652bbec/chrome/app/nibs/BookmarkBar.xib
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.h
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view_unittest.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa_unittest.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm
[modify] https://crrev.com/67b81bc38c7e2164bc77f0c00d11d26d620edc18/chrome/browser/ui/cocoa/browser_window_controller.mm
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 6 2017 

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

commit e4e3e7e4169e07176f674d94bbcc99f2392e927d
Author: lgrey <lgrey@chromium.org>
Date: Thu Apr 06 19:18:32 2017

[Mac] Bookmark button cell updates

This is extracted from the bookmark bar refactor.

Changes:
- Changing a cell's node now updates the title
- A new class method for calculating a cell's size without
instantiating one.

BUG= 648560 

Review-Url: https://codereview.chromium.org/2794123002
Cr-Commit-Position: refs/heads/master@{#462577}

[modify] https://crrev.com/e4e3e7e4169e07176f674d94bbcc99f2392e927d/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h
[modify] https://crrev.com/e4e3e7e4169e07176f674d94bbcc99f2392e927d/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
[modify] https://crrev.com/e4e3e7e4169e07176f674d94bbcc99f2392e927d/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm
Project Member

Comment 9 by bugdroid1@chromium.org, May 1 2017 

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

commit 2644729cb7722a702a76cc2758d0ce372e1e6f92
Author: lgrey <lgrey@chromium.org>
Date: Mon May 01 18:36:01 2017

Yes, with RTL thrown in, since this is specifically designed to make it almost free.

Two major things here:
- The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view.
- Removed a bunch of layout-related code that's no longer necessary

BUG= 648560 

Review-Url: https://codereview.chromium.org/2751573002
Cr-Commit-Position: refs/heads/master@{#468364}

[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge_unittest.mm
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm
[modify] https://crrev.com/2644729cb7722a702a76cc2758d0ce372e1e6f92/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
Project Member

Comment 10 by bugdroid1@chromium.org, May 1 2017 

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

commit 27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994
Author: sky <sky@chromium.org>
Date: Mon May 01 20:28:29 2017

Revert of [Mac] Refactor bookmark bar controller (patchset #10 id:180001 of https://codereview.chromium.org/2751573002/ )

Reason for revert:
Reverting as likely caused msan failurs. See https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FMac_ASan_64_Tests__1_%2F29660%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FBookmarkFolderAppleScriptTest.DeleteBookmarkItems%2F0 for example:

==77706==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000445b40 at pc 0x0001156ab4fe bp 0x7fff5c399fd0 sp 0x7fff5c399fc8
READ of size 8 at 0x603000445b40 thread T0
    #0 0x1156ab4fd in -[BookmarkBarController applyLayout:animated:] ??:0:0
    #1 0x1156a9db1 in -[BookmarkBarController rebuildLayoutWithAnimated:] ??:0:0
    #2 0x1156acf25 in -[BookmarkBarController nodeRemoved:parent:index:] ??:0:0
    #3 0x1127774c7 in bookmarks::BookmarkModel::RemoveAndDeleteNode(bookmarks::BookmarkNode*) ??:0:0
    #4 0x112776d40 in bookmarks::BookmarkModel::Remove(bookmarks::BookmarkNode const*) ??:0:0
    #5 0x10607a64c in (anonymous namespace)::BookmarkFolderAppleScriptTest_DeleteBookmarkItems_Test::RunTestOnMainThread() ??:0:0
    #6 0x10d1f6ac3 in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() ??:0:0
    #7 0x10bb87dc3 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() ??:0:0
    #8 0x10bb851e6 in ChromeBrowserMainParts::PreMainMessageLoopRun() ??:0:0
    #9 0x1077b02dd in content::BrowserMainLoop::PreMainMessageLoopRun() ??:0:0
    #10 0x1084ba8a3 in content::StartupTaskRunner::RunAllTasksNow() ??:0:0
    #11 0x1077abc3a in content::BrowserMainLoop::CreateStartupTasks() ??:0:0
    #12 0x1077b94ec in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) ??:0:0
    #13 0x1077a40b5 in content::BrowserMain(content::MainFunctionParams const&) ??:0:0
    #14 0x10b6a9604 in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) ??:0:0
    #15 0x10b6ab3c7 in content::ContentMainRunnerImpl::Run() ??:0:0
    #16 0x111afc88b in service_manager::Main(service_manager::MainParams const&) ??:0:0
    #17 0x10b6a8ff4 in content::ContentMain(content::ContentMainParams const&) ??:0:0
    #18 0x10d1f5c4f in content::BrowserTestBase::SetUp() ??:0:0
    #19 0x10ba09201 in InProcessBrowserTest::SetUp() ??:0:0
    #20 0x10ed76b2f in testing::Test::Run() ??:0:0
    #21 0x10ed78b43 in testing::TestInfo::Run() ??:0:0
    #22 0x10ed79e86 in testing::TestCase::Run() ??:0:0
    #23 0x10ed8cfb6 in testing::internal::UnitTestImpl::RunAllTests() ??:0:0
    #24 0x10ed8c588 in testing::UnitTest::Run() ??:0:0
    #25 0x10ba51d3e in base::TestSuite::Run() ??:0:0
    #26 0x10b6fc3ac in ChromeTestSuiteRunner::RunTestSuite(int, char**) ??:0:0
    #27 0x10d2c7991 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) ??:0:0
    #28 0x10b6fc203 in main ??:0:0
    #29 0x7fff8a0125fc in start ??:0:0

Original issue's description:
> Yes, with RTL thrown in, since this is specifically designed to make it almost free.
>
> Two major things here:
> - The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view.
> - Removed a bunch of layout-related code that's no longer necessary
>
> BUG= 648560 
>
> Review-Url: https://codereview.chromium.org/2751573002
> Cr-Commit-Position: refs/heads/master@{#468364}
> Committed: https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0ce372e1e6f92

TBR=ellyjones@chromium.org,avi@chromium.org,lgrey@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 648560 

Review-Url: https://codereview.chromium.org/2853123002
Cr-Commit-Position: refs/heads/master@{#468398}

[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge_unittest.mm
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm
[modify] https://crrev.com/27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
Project Member

Comment 11 by bugdroid1@chromium.org, May 3 2017 

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

commit cdde49adc3b921524027132523cb538bfa670cd4
Author: lgrey <lgrey@chromium.org>
Date: Wed May 03 15:29:36 2017

[Reland][Mac] Refactor bookmark bar controller

Fixed MSan issue (https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FMac_ASan_64_Tests__1_%2F29660%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FBookmarkFolderAppleScriptTest.DeleteBookmarkItems%2F0)

See delta between patchsets 2 and 3 for the fix

Original description:
Yes, with RTL thrown in, since this is specifically designed to make it almost free.

Two major things here:
- The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view.
- Removed a bunch of layout-related code that's no longer necessary

BUG= 648560 

Review-Url: https://codereview.chromium.org/2751573002
Cr-Commit-Position: refs/heads/master@{#468364}
Committed: https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0ce372e1e6f92

patch from issue 2751573002 at patchset 180001 (http://crrev.com/2751573002#ps180001)

Review-Url: https://codereview.chromium.org/2853373002
Cr-Commit-Position: refs/heads/master@{#468981}

[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge_unittest.mm
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm
[modify] https://crrev.com/cdde49adc3b921524027132523cb538bfa670cd4/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
Project Member

Comment 12 by bugdroid1@chromium.org, May 3 2017 

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

commit e85d0513cdbaeeff936cc30f1d73ae9887d1df43
Author: lgrey <lgrey@chromium.org>
Date: Wed May 03 22:07:57 2017

[Mac] Reverse bookmark buttons and menus in RTL (take 2)

LTR image diffs show this as:
- Identical to HEAD in retina
- In non-retina, some bookmarks at the width limit show an additional character vs. HEAD. I think this is more true to the spec and is closer to Views (though it's hard to compare directly since views fades instead of cutting off with an ellipsis)

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

BUG= 648561 ,  648560 

Review-Url: https://codereview.chromium.org/2845003003
Cr-Commit-Position: refs/heads/master@{#469157}

[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/gradient_button_cell.h
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/gradient_button_cell.mm
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/l10n_util.h
[modify] https://crrev.com/e85d0513cdbaeeff936cc30f1d73ae9887d1df43/chrome/browser/ui/cocoa/l10n_util.mm
Project Member

Comment 13 by bugdroid1@chromium.org, May 8 2017 

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

commit c4a9fa7f810710c0872eb72826916bf9c060063a
Author: lgrey <lgrey@chromium.org>
Date: Mon May 08 17:56:08 2017

[Mac] Reverse bookmark bar overflow chevron in RTL

BUG= 648560 

Review-Url: https://codereview.chromium.org/2862903002
Cr-Commit-Position: refs/heads/master@{#470046}

[modify] https://crrev.com/c4a9fa7f810710c0872eb72826916bf9c060063a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm

Comment 14 by kmaoy@google.com, Jun 30 2017

Components: -UI>Localization
Labels: Needs-TestConfirmation
Status: Unconfirmed (was: Started)
This  is not a localization bug. Can't confirm that it's still relevant to the current English UI.

Comment 15 by pnangunoori@chromium.org, Jul 3 2017

Labels: -Needs-TestConfirmation M-61 OS-Windows
Status: Untriaged (was: Unconfirmed)
Tested on Chrome Stable #59.0.30171.115 and Canary #61.0.3147.0 on Windows 10 and Mac 10.12.5 and issue is reproducible. 
 
This is issue us reproducible from M-45 #45.0.2454.101. Marking it as untriaged so that issue gets addressed.
 
Note: In Ubuntu 14.04, Bookmarks are displayed in RTL format.
 
Thanks.

Comment 16 by lgrey@chromium.org, Jul 10 2017

Status: Started (was: Untriaged)
This is fixed but behind a flag (enable chrome://flags#mac-rtl and relaunch MacOS in Hebrew or Arabic to test.)

Moving back to "Started" until it's enabled.

Comment 17 by markchang@chromium.org, Sep 19

Status: Archived (was: Started)
►

Sign in to add a comment