514462 - page titles are styled as URLs in location bar autocomplete (awesomebar styling is broken)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

The :first-line rule was somehow broken by bug 513949 ( https://hg.mozilla.org/mobile-browser/rev/73ed6ccfc7f7 ), presumably because we're no longer reflowing at the right time. Looks like a layout bug to me, but perhaps we'll have to

Comment 1

[:Gavin Sharp [email: [e-mail]]]

...work around it.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: workaround

gavin, I've encoutered this during the awesomebar style refactoring (see 485930#c13).

We can do as in the attachment but it's a bit dirty...

Comment 3

[Stuart Parmenter]

ergh, i really dislike adding these.  can you explain what is causing it to be needed?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: workaround-2

(In reply to comment #3)
> ergh, i really dislike adding these.  can you explain what is causing it to be
> needed?

As gavin suggested it's probably a (reflow) layout bug that happen when the box is  'hidden' and the :first-line css rule is applied on it. I can't say more on it for now.

See the attachment for an other (less dirty) workaround...

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 400355 [details] [diff] [review]
workaround-2

This a better workaround. I wonder if it could help in awesomebar opening speed (or at least the perception)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 400355 [details] [diff] [review]
workaround-2

This is functionally equivalent to just removing the call entirely, since invalidate() returns early if _popupOpen is false. Given that, it relies on the controller invalidating us twice (once before the popup is open, and once after for the second chunk), which will cause bustage if we ever change to getting results in one chunk.

I tried changing nsAutoCompleteController::ProcessResult to open before invalidating, which would also allow us to get rid of the invalidate() call in openAutocompletePopup entirely, but that doesn't fix this bug, because the invalidate is still called synchronously before the popup's been opened.

For now, I think we probably just want the setTimeout() patch to delay the first invalidate() until the popup is fully open.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> (From update of attachment 400355 [details] [diff] [review])

> For now, I think we probably just want the setTimeout() patch to delay the
> first invalidate() until the popup is fully open.

Would Util.executeSoon work with less timer overhead? Or do we need and actual delay?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-1

Ok, this wip resolved :
 * bug 514462 - (this bug)
 * bug 507035
 * bug 514285 - (with a liggle crop="center|middle" add for the bookmarks)

and maybe it have some perfs effects, which i hope to be in the right way...


I still need to do the winmo css if we want to go in this direction!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Would Util.executeSoon work with less timer overhead?

Should work as well, but without having looked into it much, I'm not confident that setTimeout's "timer overhead" outweighs thread-manager-from-JS's xpcom/xpconnect overhead.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: setTimeout workaround patch

I think we might just want to take this patch for now, and move the WIP patch to a followup bug?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 401107 [details] [diff] [review]
Patch

I would rather land a full patch that resolved our problems without a workaround.

As you have say, if it can land soon it's ok, otherwise I'm agree with the setTimeout (I'm bored to see theses blue lines each time...)

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

* replace <xul:caption/> by <hbox><image/><label/></hbox>

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 401844 [details] [diff] [review]
Patch v0.2

>diff -r cafbf066fef9 chrome/content/bindings.xml

>   <binding id="popup_autocomplete_result">
>-    <content align="center">
>-      <xul:image class="autocomplete-item-image" src="" xbl:inherits="src"/>
>-      <xul:description flex="1" class="autocomplete-item-desc" xbl:inherits="xbl:text=value"></xul:description>
>-      <xul:label class="autocomplete-item-tags" xbl:inherits="favorite,xbl:text=tags"></xul:label>
>+    <content orient="vertical">
>+      <xul:hbox class="autocomplete-item-name" align="center" xbl:inherits="tags, favorite" mousethrough="always">

use autocomplete-item-label instead?

you can just inherit the "value" and "image" attributes directly to avoid having to change the other code. 

>diff -r cafbf066fef9 themes/hildon/browser.css

> autocompleteresult {
>-  padding: 0.5mm 0.2mm;
>+  color: black;
>+  padding: 1.5mm 1mm;

should have a match background-color: white, I guess?

looks good with those addressed, but I'd like to take a look at the revised patch.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Address comments

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 401860 [details] [diff] [review]
Address comments


oups.
* Address comments
* Fix a bug found with the change of the caption vs hbox in the .noresults class

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 401860 [details] [diff] [review]
Address comments


>diff -r eb51ff1d008e themes/hildon/browser.css

>+autocompleteresult.noresults > .autocomplete-item-label[favorite="true"],
>+autocompleteresult.noresults > .autocomplete-item-label[favorite="true"]:after {
>+  content: none;
>+  background: none;
>+}

Hmm, seems like we should actually be clearing out the attributes in this case rather than relying on CSS.

>+autocompleteresult.allbookmarks {
>+  -moz-box-pack: center;
>+  background: #E9E9E9 url(chrome://browser/skin/images/arrowright-16.png) no-repeat 98% 50%;
>+}

Could use a relative URI here (we do that elsewhere in the file).

I think some of these rules belong in the /content/ stylesheet, but we can fix that up in a separate bug.

r=me with those addressed.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Address comments

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/ee72ab39b952

Comment 20

[Aakash Desai [:aakashd]]

verified FIXED (using https://verisign.com) on build:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2a2pre) Gecko/20090922
Fennec/1.0b4pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2a2pre) Gecko/20090922
Fennec/1.0b4pre