Closed Bug 218528 Opened 21 years ago Closed 20 years ago

Personal Toolbar is empty when the UI is right-to-left

Categories

(Core :: Layout: Text and Fonts, defect, P2)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta1

People

(Reporter: tsahi_75, Assigned: mkaply)

References

Details

(Keywords: fixed1.5)

Attachments

(4 files, 3 obsolete files)

screenshot of problem in ar-SA language pack
(deleted), image/jpeg
Details
What jag said
(deleted), patch
janv
: review+
jag+mozilla
: superreview+
asa
: approval1.5+
Details | Diff | Splinter Review
Added small fix: always measure the toolbar's width after we collapse the chevron
(deleted), patch
Details | Diff | Splinter Review
minor change (comment 38)
(deleted), patch
asaf
: review+
asaf
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.4) Gecko/20030624

i've been making hebrew (and thus, RTL) language packs for mozilla since version
0.9.4, and never seen this: when the pack is activated, and the interface is
flipped to RTL, the personal toolbar is emptied, and all the bookmarks on it are
only accessible from the small arrow at the end of the bar, as if the bar was
full of other bookmarks.

Reproducible: Always

Steps to Reproduce:
1. aligning the interface to the right: add these lines to the file intl.css, in
the locale\en-US\global, in the en-US.jar file (the language pack file, in the
chrome folder):

/*make UI RTL */

window,dialog,wizard,page { direction: rtl; }

menu { direction: rtl; }

outliner { direction: rtl; }

/*
 * make sure search from address bar remains in RTL
 */

#urlbar .autocomplete-search-engine
{
direction: rtl !important;
}

/*
 * keep Composer <HTML> Source tab LTR
 */

#content-source,
#doctype-text { direction: ltr; }

2. start mozilla
3. make sure you have some bookmarks in the personal toolbar.
3. see empty toolbar, but bookmarks are present in the menu opened by the arrow
at the (now) left end of the bar.

i didn't really do step 1, i added these lines to the intl.css file of the
hebrew pack and installed it. the effect should be the same, but if it isn't,
i'll post a link to the pack (it's not on the 'net yet).
Actual Results:  
as described

Expected Results:  
like in mozilla 1.3 and earlier, the bookmarks should populate the toolbar,
although in reverse order compared to a LTR interface.

this did not happen on a similar pack for version 1.3/1.3.1.

this is a major usability problem, so i would really appreciate a quick fix.
BiDi language packs have a lot of other problems, but they are almost all
cosmetic, and don't affect the functionality like this one does.
Flags: blocking1.5?
Flags: blocking1.4.2?
this screenshot was sent to me by the translator to arabic (ar-SA). look at the
bookmarks hanging from the arrow at the left end, while the toolbar is empty.
Sounds like a regression from bug 66919.
This isn't a bug in bi-di, I think, but rather an apps bug so we need someone
that knows the new overflow code to take a look at it. Who can help? Pch? Jan? Jag?
bug 66919 was fixed in April, but the problem didn't exist in Mozilla 1.3.1
which was released in May.
on the other hand, as much as i can see, the fix for bug 66919 was not inserted
to 1.3.1, so my last comment may not be relevant.
I checked this problem in windows 2000 and Linux . Can anyone change the OS type
to All
OS: Windows XP → All
Hardware: PC → All
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksMenu.js#736

736       if (button.boxObject.x + button.boxObject.width + chevronWidth > width) {

where |width| is toolbar.x + toolbar.width, or put in English, the x coordinate
of the right hand side of the bookmarks toolbar.

Because of RTL, the bookmark buttons are placed at the right hand side of the
toolbar, then we see if their right hand side's x coord. is to the right of the
chevron's left hand side x coord., which this formula assumes is at the right
hand side of the toolbar.

That assumption of course is false, as is the assumption in how we check to see
if the bookmark button would overlap with (or rather, push out) the chevron. A
more directionally independent way would be to sum the bookmark buttons' widths
and margins and compare that against the toolbar's width - the chevron's width
and margin.

Or something like that. CC'ing some people who hopefully understand what I'm
talking about and maybe know more than I do to know how to completely fix this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch What jag said (deleted) — — Splinter Review 
btw, outliners went out before 1.0, no?
Attachment #132053 - Flags: superreview?(jag)
Attachment #132053 - Flags: review?(varga)
i think i was once told to use "trees" instead of "outliners", but i didn't see
any difference. i really don't know much about this stuff.
Comment on attachment 132053 [details] [diff] [review]
What jag said

sr=jag
Attachment #132053 - Flags: superreview?(jag) → superreview+
Comment on attachment 132053 [details] [diff] [review]
What jag said

a=asa (on behalf of drivers) for checkin to the 1.5 branch (pending an r=).
Please add the fixed1.5 keyword when this fix lands on the branch. Time is
short for 1.5.
Attachment #132053 - Flags: approval1.5+
Attachment #132053 - Flags: review?(varga) → review+
Otherwise, when you resize with the chevron showing you'll take the chevron's
width into account twice.

This is what I checked in on both trunk and 1.5 branch.
jag, hey, you only asked me to fix one existing bug :-P

tsahi, styling outliner will have no effect because we don't have any;
styling menu should be unnecessary too.
Keywords: fixed1.5
Flags: blocking1.5?
this looks fixed in 1.5rc2.
not quite fixed: in 1.5final, if you drag a link from the address bar or another
bookmark from the personal toolbar, and drop it on the toolbar, it doesn't
behave as expected:
1. if it's dropped between two existing bookmarks, the new bookmark will be to
the left of the left-most of the two.
2. if it's droppen to the right of the right-most existing bookmark (i.e. you
want it to be the first bookmark), it will become the second bookmark.
3. if it's dropped to the left of the left-most exising bookmark (i.e. you want
it to be the last bookmark) the thin line that marks its expected location while
you hold it is between the existing last bookmark and the one before it, as if
it will be dropped before the current last bookmark, but when you release it, it
becomes the last bookmark.

in short, the new bookmark is added one place to the left of it's expected
place.  this is similar to bug 176244.
by writing comment 15, i was hoping someone will fix it.
This looks safe enough for 1.4.2 if it is wanted...
Flags: blocking1.4.2? → blocking1.4.2+
michael: if it's still relevant, then yes, it will be nice.
Attached patch Addressing Drag & Drop problem (comment 15) (obsolete) (deleted) — — Splinter Review 
Patching the last problem in this bug (see comment 15 for details).
Comment on attachment 151570 [details] [diff] [review]
Addressing Drag & Drop problem (comment 15)

requesting r+sr

If someone is testing the patch, make sure to make chrome.
Attachment #151570 - Flags: superreview?(mkaply)
Attachment #151570 - Flags: review?(smontagu)
I understand what you are doing here, but I am worried if the values of
DROP_BEFORE and DROP_AFTER change someday, we will have a problem.

Can we make the assignment explicit? or perhaps add a macro somewhere called
"INVERT_DROP"
(In reply to comment #21)
> I understand what you are doing here, but I am worried if the values of
> DROP_BEFORE and DROP_AFTER change someday, we will have a problem.
> 
> Can we make the assignment explicit? or perhaps add a macro somewhere called
> "INVERT_DROP"

1. mmm.. the values are very logical, so i don't beileive the wil be changed BUT
2. i can replace the a=-a width a condiftion (IF... AFTER...ELSE IF BEFORE...)
Attached patch patch (obsolete) (deleted) — — Splinter Review 
Same result, but more clear (and without being dependent on the values of
DROP_*)
Attachment #151570 - Attachment is obsolete: true
Attachment #151586 - Flags: superreview?(mkaply)
Attachment #151586 - Flags: review?(smontagu)
Attachment #151570 - Flags: superreview?(mkaply)
Attachment #151570 - Flags: review?(smontagu)
Comment on attachment 151586 [details] [diff] [review]
patch

r=smontagu, but you need moa from Neil or Jag or Dean (assuming that
http://mozilla.org/owners.html is up to date wrt XPApps)
Attachment #151586 - Flags: review?(smontagu) → review+
Attachment #151586 - Flags: superreview?(mkaply) → superreview?(jag)
Attached patch Final patch (obsolete) (deleted) — — Splinter Review 
Simon pointed me to some places where the patch cause regressions (meaning the
problem it had fixed, appeared in other places). This patch change the behavior
only for the issue we have (i.e. doesn't change it for all drag & drop code,
only for drop on PersonalBar bookmarks buttons).

Thank you Simon for great help.
Attachment #151586 - Attachment is obsolete: true
Attachment #151618 - Flags: superreview?(jag)
Attachment #151618 - Flags: review?(smontagu)
Attachment #151586 - Flags: superreview?(jag)
Attachment #151618 - Flags: review?(smontagu) → review+
Priority: -- → P2
Target Milestone: --- → mozilla1.8alpha2
I seem to remember Jan specifically changed DROP_* to be more logical.
(In reply to comment #26)
> I seem to remember Jan specifically changed DROP_* to be more logical.

But it caused a regression in the drop code (only for personal bar, only on drop
event, only on toolbar buttons), odd that this happens only in that case.
Flags: blocking1.8a2?
Are we fixing the right thing here then? How come other drag and drop code
doesn't have this problem? Or do they all do orientation checks?
(In reply to comment #28)
> Are we fixing the right thing here then? How come other drag and drop code
> doesn't have this problem? Or do they all do orientation checks?

Most of the darg code is about vertical dragging (menu items etc.), this doesn't
need to be changed for RTL.

The only weird thing - the line that points to where the bookmark will get into,
was drawn in the right place (not in the meaning of riht/left :-) ) BEFORE
patching, so if we just change getBTOrientation to always invert its result for
bookmark-toolbar-buttons, it will fix the drop problem, but it will also cause a
new one (but this time, in the drag case).


And yep, w do fix the right thing :-)
So how come the line is/was being drawn in the right place?
(In reply to comment #30)
> So how come the line is/was being drawn in the right place?

That's because of this:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksMenu.js#618
(remember that orientaion is wrong for rtl horizontal)

left is the start for ltr, but it's the end for rtl (same principle for right),
ofcousr we can start IFing everywhere, but we realy don't need this.
Flags: blocking1.8a2? → blocking1.8a2-
Hmm, I guess in a clean world, that code in onDragSetFeedback would check for
RTL and make DROP_BEFORE set dragover-right and DROP_AFTER set dragover-left.
(In reply to comment #32)
> Hmm, I guess in a clean world, that code in onDragSetFeedback would check for
> RTL and make DROP_BEFORE set dragover-right and DROP_AFTER set dragover-left.

The question is if we need this, when the *drag* code already gives the expected results (IMO, we want 
RTL/LTR checks as lees as possible).
Target Milestone: mozilla1.8alpha2 → mozilla1.8beta
Well, we may not need it, but it seems wrong to depend on the current behaviour.

In RTL, "before" is on the right hand side of the element, so the fact that we
get "after" when we're on the right hand side and "before" when we're on the
left hand side seems like a bug to me. As a developer, I wouldn't expect that.
Of course, I'd learn to live with it, but I'd always shake my head when I'd have
to work with that code again.
The border is drawn on the physical side of the button, so BEFORE is left and
AFTER is right. We don't (yet?) have a way of drawing logical borders.
Neil: "before" and "after" are concepts that apply equally well when in RTL
mode, except that in RTL mode "before" should map to "draw border on right" and
"after" should map to "draw border on left".
Comment on attachment 151618 [details] [diff] [review]
Final patch

sr=jag to just get this fixed. It's not the solution I was looking for, but
it's good enough for now.
Attachment #151618 - Flags: superreview?(jag) → superreview+
Comment on attachment 151618 [details] [diff] [review]
Final patch

Neil just pointed out that we should probably do the target.localName check
before the direction check, just to make it clear that we're only doing this
for toolbarbuttons and only if the PT direction is RTL.

He also mentioned the comment could specify why it needs to be inverted. E.g.:
// because "before" (to the left) on the screen translates to "after" in the
collection of items.
Attached patch minor change (comment 38) (deleted) — — Splinter Review 

        Attachment #151618 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 153927 [details] [diff] [review]
minor change (comment 38)

Moving r/sr

        Attachment #153927 -
        Flags: superreview+

        Attachment #153927 -
        Flags: review+

    
    

    
  
Simon: can you it checkin?

    
    

    
  
Fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
/me beats smontagu

You checked in the wrong patch or "both patchs", don't know what... now we have
the invert code in both getBTOrientation and in onDrop.

can you please check it in again? (and remove the changes from
getBTOrientation), thanks.


    
    

    
  
Oops, sorry about that. I backed out the wrong part of the checkin.

    
  
Blocks: 434749

    
  
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: