Invalid form popup should take into account rtl

RESOLVED FIXED in Firefox 4.0b10

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete, polish, rtl})

Trunk
Firefox 4.0b10
dev-doc-complete, polish, rtl
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(1 attachment, 2 obsolete attachments)

6.76 KB, patch
Neil Deakin (not available until Aug 9)
: review+
smontagu
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Currently, invalid form popup points to the left of the element (maybe the right with RTL, I don't know). We should probably make it point to the middle to prevent potential issues with RTL and to prevent positions issues when the elements are completely restyled and have a padding like in this screenshot: http://fredericiana.com/wp-content/uploads/2010/12/html5-feedback-forms-1.jpg
Thanks for filing this followup bug for me, and apologies that I thought it was the same problem as bug 616607!
(Assignee)

Comment 2

7 years ago
Actually, I don't think we should point to the middle but we can just take into account borders and padding. It's quite simple (I already have a patch doing that). In addition, we should make sure to point to right of the element if it's in RTL mode (it's currently not working nicely in RTL).

However, Neil, for unknown reasons, if I do openPopup(element, "after_start", w, h) with w and h being negative values, the arrow of the arrow panel disappeared. Is that a simple bug or there are some reasons by design?
Keywords: polish, rtl
Summary: Invalid form popup should probably point to the middle of the element → Invalid form popup should take into account padding, border and rtl
(Assignee)

Comment 3

7 years ago
Ehsan, do we agree that it would be better to have invalid form popup showing at the right of the element (near the cursor) when in RTL mode? Though, the arrow will still be at the left of the popup. Would that be all right?
Given that I do not use RTL, I prefer not to do wrong assumptions here.
(Assignee)

Comment 4

7 years ago
Created attachment 500323 [details] [diff] [review]
Patch v1

With this patch, the popup has now an offset depending on the border and the padding so the arrow will point just below the content.
For checkbox and radio buttons, the arrow will point at the center.

When the element is in RTL, the arrow should be on the right of the popup and point on the right of the element (or the center).
However, I think there is a bug: sometimes the panel is positioned as asked but the arrow hasn't the correct position. Enn, let me know if you have any idea why I have this bug. This only happen in RTL when I ask to have the arrow on the right (the arrow is on the left but the panel is positioned to have it on the right).

Simon, could you tell me if what is done for RTL is what you would expect?
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #500323 - Flags: review?(enndeakin)
Attachment #500323 - Flags: feedback?(smontagu)
(Assignee)

Comment 5

7 years ago
The page in the URL field can be used to test this patch.
Note that the bug mentioned in comment 4 can be seen by switching to RTL with the checkbox then submitting the form: the bug will happen for all text fields but not for checkbox and radio buttons.
Whiteboard: [needs-review]
Are you testing this on a page that has rtl direction or when the browser UI has rtl direction? Or, better, both?
(Assignee)

Comment 7

7 years ago
Gasp, the browser UI direction changes how the arrow panel behave :-/

So, with the current patch and:
 1. brower UI is RTL but not the content: the panel is positioned at the right and the arrow is on the left but should be on the right ;
 2. browser UI is RTL and the content is RTL: the panel is on the left and the arrow is on the left ;
 3. browser UI is not RTL but the content is: the panel is positioned at the right and the arrow is on the left but should be on the right ;
 4. browser UI is not RTL and content is not RTL: the panel is on the left and the arrow is on the left.

So what happens here is that my patch is trying to detect if we are in RTL mode and change how the panel should appear but I guess the panel code does the same so in some situations it doesn't behave as expected.

I would say that the arrow should take the direction from the content and not the browser UI given that having an RTL popup in a LTR page might be weird. In addition, the content can redefine the message inside the popup which can lead to other issues :(

For the bug, Neil, I think it is persistent. Obviously cases 1 and 4 should have the panel at the right and the arrow at the right.

Maybe we should block on this for the RTL issue?
It may just be the case that the three places in nsMenuPopupFrame.cpp and the one place in popup.xml where the direction is used just need to be changed to use the anchor's direction rather than the popup's direction. Thus, the rtl parts of your patch may not be needed.
(Assignee)

Comment 9

7 years ago
This is what happens with the changes recommended in comment 8 (copied from IRC):
[16:15] < volkmar> Enn: with the browser UI in LTR, everything is fixe except a 
                   bug when the arrow should be in "bottomcenter topleft" in 
                   RTL (which is topright then) : the panel is positionned to 
                   be in topright but the arrow is in topleft
[16:15] < volkmar> as a consequence, the arrow points nowhere
[16:16] < volkmar> Enn: let me know if you need some screenshots, i doubt that 
                   my description are clear
[16:18] < volkmar> Enn: and when the browser UI is in RTL, the panels are 
                   always at the correct position but the arrow are most of the 
                   time misplaced

In addition, when the browser UI is in RTL, the notification popup direction changed but I guess this could be fixed by inverting the direction before calling openPopup?
Comment on attachment 500323 [details] [diff] [review]
Patch v1

What should happen IMO is this:

The directionality of the text in the pop-up should be inherited from the direction of the browser (so with RTL browser UI, it should appear as ".Please fill out this field" with the period on the left). This is working correctly.

The position of the pop-up and the arrow should depend only on the directionality of the field that it's attached to. The arrow should be at the "start" of the field, and the pop-up should be aligned to the "start", where "start"=left for a LTR field and right for a RTL field.

FWIW, this works correctly in trunk for the "remember password" pop-up (bug 613843 and bug 606343).
Attachment #500323 - Flags: feedback?(smontagu)
FWIW, I agree with Simon in comment 10.
(Assignee)

Updated

7 years ago
Depends on: 624151
(Assignee)

Comment 12

7 years ago
I split the bug. This will remain for the RTL issues. The original issue is now in bug 624151.

Asking a blocker status given that RTL users might have a bad experience with the current implementation of the invalid form popup.
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Updated

7 years ago
Whiteboard: [needs-review]
(Assignee)

Updated

7 years ago
Summary: Invalid form popup should take into account padding, border and rtl → Invalid form popup should take into account rtl
(Assignee)

Updated

7 years ago
Attachment #500323 - Flags: review?(enndeakin)
(Assignee)

Updated

7 years ago
Attachment #500323 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Created attachment 502267 [details] [diff] [review]
Patch v2

This patch should do what Simon and Ehsan are requesting:
The content direction of the panels is following the general UI direction but the arrow position depends on the content direction. It also prevents the workaround for the identity, password and geolocation panels direction.
Attachment #502267 - Flags: review?(enndeakin)
Attachment #502267 - Flags: feedback?(ehsan)
(Assignee)

Comment 14

7 years ago
Created attachment 502269 [details] [diff] [review]
Patch v2

Better with a real patch...
Attachment #502267 - Attachment is obsolete: true
Attachment #502269 - Flags: review?(enndeakin)
Attachment #502269 - Flags: feedback?(ehsan)
Attachment #502267 - Flags: review?(enndeakin)
Attachment #502267 - Flags: feedback?(ehsan)
(Assignee)

Updated

7 years ago
Attachment #502269 - Flags: feedback?(smontagu)
(Assignee)

Comment 15

7 years ago
Neil, you should note that there is still one bug with the arrow panels which is unrelated to the patch. If you try the the second form (in RTL) in the URL field with this patch applied, the arrow panel will not apply correctly: at the right but the arrow at the left.
(Assignee)

Updated

7 years ago
Whiteboard: [needs-review]
Comment on attachment 502269 [details] [diff] [review]
Patch v2

This looks good, modulo the issue mentioned in comment 15
Attachment #502269 - Flags: feedback?(smontagu) → feedback+
Attachment #502269 - Flags: review?(enndeakin) → review+
(In reply to comment #15)
> Neil, you should note that there is still one bug with the arrow panels which
> is unrelated to the patch. If you try the the second form (in RTL) in the URL
> field with this patch applied, the arrow panel will not apply correctly: at the
> right but the arrow at the left.

Which arrow panel are you referring to and what is the 'second form'?
(Assignee)

Updated

7 years ago
Attachment #502269 - Flags: feedback?(ehsan) → approval2.0?
(Assignee)

Updated

7 years ago
Whiteboard: [needs-review] → [needs-approval]
Attachment #502269 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs-approval] → [needs landing]
blocking2.0: ? → final+
Whiteboard: [needs landing] → [needs landing][softblocker]
The values for position should be changed to "end" and "start" values (e.g. bottomstart, topend, etc.). Either way, this needs documentation and is somewhat of a compat change.
Keywords: dev-doc-needed
(Assignee)

Comment 19

7 years ago
This patch actually breaks layout/xul/base/test/test_bug477754.xul

See: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294748904.1294753151.31939.gz#err0
(Assignee)

Updated

7 years ago
Whiteboard: [needs landing][softblocker] → [softblocker]
(Assignee)

Comment 20

7 years ago
(In reply to comment #17)
> (In reply to comment #15)
> > Neil, you should note that there is still one bug with the arrow panels which
> > is unrelated to the patch. If you try the the second form (in RTL) in the URL
> > field with this patch applied, the arrow panel will not apply correctly: at the
> > right but the arrow at the left.
> 
> Which arrow panel are you referring to and what is the 'second form'?

The RTL one.

By the way, it seems like test_bug477754.xul tests something that doesn't really apply anymore (the customize panel is centered). Do we still want this test? or I'm missing the point of it?
(In reply to comment #20)
> 
> By the way, it seems like test_bug477754.xul tests something that doesn't
> really apply anymore (the customize panel is centered). Do we still want this
> test? or I'm missing the point of it?

That test isn't testing the customize panel, it tests the underlying cause of the bug.
(Assignee)

Comment 22

7 years ago
(In reply to comment #21)
> (In reply to comment #20)
> > 
> > By the way, it seems like test_bug477754.xul tests something that doesn't
> > really apply anymore (the customize panel is centered). Do we still want this
> > test? or I'm missing the point of it?
> 
> That test isn't testing the customize panel, it tests the underlying cause of
> the bug.

So, making the anchor RTL instead of the popup fixes this test.
(Assignee)

Updated

7 years ago
Whiteboard: [softblocker] → [softblocker][passed try]
(Assignee)

Updated

7 years ago
Whiteboard: [softblocker][passed try] → [softblocker][passed try][needs landing]
(Assignee)

Comment 23

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/7fcae0c7f36a
Whiteboard: [softblocker][passed try][needs landing] → [softblocker]
Target Milestone: --- → Firefox 4.0b10
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
I don't see any obvious new API here, other than what looks like an internal IsDirectionRTL() function. Is the documentation point here to simply say the popup hangs the opposite direction for RTL locales?
(In reply to comment #15)
> Neil, you should note that there is still one bug with the arrow panels which
> is unrelated to the patch. If you try the the second form (in RTL) in the URL
> field with this patch applied, the arrow panel will not apply correctly: at the
> right but the arrow at the left.

If that bug is unrelated to this patch, does it have its own bug?
(Assignee)

Updated

6 years ago
Depends on: 626605
(Assignee)

Comment 26

6 years ago
(In reply to comment #25)
> (In reply to comment #15)
> > Neil, you should note that there is still one bug with the arrow panels which
> > is unrelated to the patch. If you try the the second form (in RTL) in the URL
> > field with this patch applied, the arrow panel will not apply correctly: at the
> > right but the arrow at the left.
> 
> If that bug is unrelated to this patch, does it have its own bug?

Sorry, I should have open a bug. It's now done: bug 626605.
But since this patch is pushed I'm trying to understand why the behavior is weird but I don't understand the arrow panel code enough yet.
(Assignee)

Updated

6 years ago
Depends on: 627146
Added a note here:

https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5#Constraint_Validation
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 28

6 years ago
Eric, I don't think the dev-doc-needed flag was for HTML5 Forms but for the XUL panel which now depends on the direction of the anchor.
Keywords: dev-doc-complete → dev-doc-needed
Oh. That wasn't clear at all from any of the discussion or the patch. Thanks for catching my error!

Updated:

https://developer.mozilla.org/en/XUL/Attribute/panel.type

Hopefully that covers it correctly; otherwise I'm stumped. :)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 30

6 years ago
(In reply to comment #29)
> Oh. That wasn't clear at all from any of the discussion or the patch. Thanks
> for catching my error!
> 
> Updated:
> 
> https://developer.mozilla.org/en/XUL/Attribute/panel.type
> 
> Hopefully that covers it correctly; otherwise I'm stumped. :)

Yes, the bug name might be confusing.
The change is correct. However, it would have been better to document that in |panel.openPopup| but I have no idea if there is such a documentation and the popup isn't at the left in LTR and at the right in RTL but it's inversed in RTL.
I added a sentence to:

https://developer.mozilla.org/en/XUL/Method/openPopup

to mention that the orientation of the panel varies by locale. I don't think this needs to be more specific...
You need to log in before you can comment on or make changes to this bug.