The contextual error message about insecure forms over HTTP inherit the content direction, while it should use the browser UI direction on RTL builds. Steps to reproduce: a. Download the attached simple user/password form. b. place the file on a server accessible by HTTP. You can use Python SimpleHTTPServer [1], but the server MUST be accessed by domain/hostname[2], as IP addresses and localhost won't trigger the message. [1] $ python -m SimpleHTTPServer [2] $ hostname

On LTR languages, the testcase above will show the expected result only on the left column (dir=ltr), while on RTL languages only the right column (dir=rtl) is featuring the expected result for these locales. Since users with RTL browser locale may access LTR websites and vice versa, the message should inherit the browser locale direction.

[Tracking Requested - why for this release]: Insecure Password Feature is enabled in FF 52. Tomer, this bug has to land and uplift to 52. Will you be able to do this in this timeframe?

Updating testcase code, so now it'd work directly on Bugzilla without the need for a local [insecure] server.

tracking for 52 as an issue with this new contextual warning

riclistitem[type=insecureWarning] {direction: rtl} doesn't seems to solve this, as it somehow transform the listitem into inline item. Then, when overriding with display:block I got it in a separated line than the <xul:image>.

Any update here?

The flags were accidentally cleared.

Restoring tracking flag.

We need to land and uplift this all the way to beta by Feb 2nd. Do you think you can do that or should we find someone else to take this bug?

Hey Tomer, do you mind if I take this bug off of your hands? If I don't hear back by Monday, I'll assume it's okay. Thanks!

Just fyi, I think what we probably want to do is intervene here: http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/toolkit/content/browser-content.js#1550 This is where, currently, we send up the direction as computed by the page up to the parent to render the autocomplete popup. We should probably intervene here if the input is one that we want to show logins / the insecure context warning on. The question is - how best to determine if the input is one that we want to show logins / the insecure context warning on? Is there a good API to use for this?

The autocomplete suggestion box inherits its direction from the parent element, which may be good for the suggestions, since the input direction can predicts the preferred content direction but not for the message, which should inherit the UI language direction. Please keep in mind that in the meantime we've placed a temporary workaround in the Hebrew locale, so while the the string itself appears as expected at least on the Hebrew locale (but not right-aligned and the lock icon still appears in the wrong side).

My posted solution here is what I think is the simplest thing we can safely land and uplift to beta. I suspect the more "correct" solution is altering nsIAutoCompleteController and nsIAutoCompleteResult so that an nsIAutoCompleteResult can express a preference for whether or not the input elements text direction should be used or not, and then expose that through nsIAutoCompleteController for browser-content.js to check out.

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review109650 ::: toolkit/content/browser-content.js:1553 (Diff revision 2) > + let dir = "inherit"; > + if (!isLoginResults) { > + dir = window.getComputedStyle(element).direction; I don't understand why the behaviour would need to differ based on whether the results are from login manager since they use the same bindings other than the warning item. Is this just to avoid affecting form history/datalist and adding risk? Is the problem not noticeable because non-login ones don't have icons?

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review109650 > I don't understand why the behaviour would need to differ based on whether the results are from login manager since they use the same bindings other than the warning item. Is this just to avoid affecting form history/datalist and adding risk? Is the problem not noticeable because non-login ones don't have icons? I really don't know what the best approach is here. I'll happily just do what seems most sensible here - does that just mean ensuring that the contextual feedback richlistitem inherits the UI's direction?

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review109650 > I really don't know what the best approach is here. I'll happily just do what seems most sensible here - does that just mean ensuring that the contextual feedback richlistitem inherits the UI's direction? I'm very confused about what is expected here since there seems to be conflicting information between this bug, bug 1106235, and bug 649840. What is expected for each of the following rows on both an LTR field and RTL field: * Warning with lock icon * Login with key icon with a LTR username * Login with key icon with a RTL username * Future footer "View Saved Logins"

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review109650 > I'm very confused about what is expected here since there seems to be conflicting information between this bug, bug 1106235, and bug 649840. > > What is expected for each of the following rows on both an LTR field and RTL field: > * Warning with lock icon > * Login with key icon with a LTR username > * Login with key icon with a RTL username > * Future footer "View Saved Logins" Here is a spreadsheet where we can figure out what to do: https://docs.google.com/spreadsheets/d/1l0dprMJ5tiGTyI6J8KWFCpx3pEX18YzdyJNfm-jWhN8/edit#gid=0

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #18) > What is expected for each of the following rows on both an LTR field and RTL > field: > * Warning with lock icon > * Login with key icon with a LTR username > * Login with key icon with a RTL username > * Future footer "View Saved Logins" I am a native RTL reader. Below are my two cents: Warning with lock, future footer both should be written in RTL so has to be with dir=rtl. RTL username with a key icon, LTR username with a key icon both should have the icon on the right side, but maybe dir=auto would be more appropriate for the content itself (but not the icon).

Thanks tomer. How about you, ehsan? Your 2c?

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review109650 > Here is a spreadsheet where we can figure out what to do: https://docs.google.com/spreadsheets/d/1l0dprMJ5tiGTyI6J8KWFCpx3pEX18YzdyJNfm-jWhN8/edit#gid=0 After discussions with Ehsan and Tomer we're going to implement what's in the spreadsheet now.

So there's good news and bad news. The good news is that we already satisfy Cases 1, 2, 3 and 4 from the spec. We're already shipping that. \o/ The bad news is about "auto". "auto" operates by checking the characters that have been placed in the element and making a judgement call on whether or not render them ltr or rtl. For a login <input> with dir="auto", we might have autocomplete entries that are both ltr and rtl. The spec, in case 5 and 6, wants us to honor the "auto" behaviour, and render each entry in the autocomplete popup with auto. But there's a problem. dir="auto" is something that can only be set as an attribute, and cannot be set with CSS. We use the autocomplete-rich-result-popup binding for the popup, and in that binding, we have this code for appending the entries: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/toolkit/content/widgets/autocomplete.xml#1320 I have experimented with having this first check to see if there's a dir attribute on the panel, and then applying that (and falling back to style.direction), but there's an added wrinkle; the autocomplete-richlistitem's are kinda complicated. In order to get the <xul:description> to flip to RTL, I'd need the _outer_ <xul:description> to flip to RTL, since we need the inner <xul:description> to be small in order to deal with text overflow (I believe). And these autocomplete-richlistitem's are also used by the AwesomeBar for the results that show up in the panel. I'm very reluctant to attempt to shoehorn in any fixes to this since it has the possibility of affecting the AwesomeBar. Because we already support cases 1, 2, 3 and 4, and because of the risk of attempting to fix cases 5 and 6, I think we should WONTFIX this for 52, at the very least. tanvi, do you concur?

Redirecting comment 23 ni? to pdol.

(In reply to Mike Conley (:mconley) from comment #23) > Because we already support cases 1, 2, 3 and 4, and because of the risk of > attempting to fix cases 5 and 6, I think we should WONTFIX this for 52, at > the very least. tanvi, do you concur? This approach makes sense to me. It's likely not the end of the world if 52 doesn't have use cases 5 and 6 covered properly.

Are there any dependency bugs we need to file and do first?

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #26) > Are there any dependency bugs we need to file and do first? Marking as P2... which in this case means we want this in FF 53 and 54.

Comment on attachment 8831515 [details] Bug 1330597 - Login autocomplete should use UI text direction, and not contents text direction. https://reviewboard.mozilla.org/r/108074/#review113070 Obsolete patch

So one thing I'll note with this bug is that dir="auto" does not work at all for autocomplete in general, let alone login manager autocomplete. I have filed bug 1341909 for this, but since we've been shipping that bug for a while (we've never supported dir="auto" with autocomplete in the way described in this bug, as far as I can tell) I think we can pump the brakes here. tanvi, do you concur?

Redirecting to pdol, as tanvi is going on leave soon. Essentially, the question: can this be retargeted and given normal priority, due to the fact that we've never shipped a proper dir="auto" solution for autocomplete popups?

(In reply to Mike Conley (:mconley) from comment #30) > Essentially, the question: can this be retargeted and given normal priority, > due to the fact that we've never shipped a proper dir="auto" solution for > autocomplete popups? FWIW I think so.

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #30) > Redirecting to pdol, as tanvi is going on leave soon. > > Essentially, the question: can this be retargeted and given normal priority, > due to the fact that we've never shipped a proper dir="auto" solution for > autocomplete popups? Yes, it looks like Bug 1341909 was set to P3 which seems like the appropriate priority. I suggest we set this one the same, P3.

Thanks. I'm not sure we really need two bugs as bug 1341909 may do all the work but marking as P3.