Closed Bug 514462 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b4
Tracking Status
fennec 1.0+ ---

People

(Reporter: Gavin, Assigned: vingtetun)

References

Details

(Keywords: regression)

Attachments

(1 file, 7 obsolete files)

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
tracking-fennec: --- → ?
Summary: page titles are styled as URLs in location bar autocomplete → page titles are styled as URLs in location bar autocomplete (awesomebar styling is broken)
...work around it.
Attached patch workaround (obsolete) — Splinter Review
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...
ergh, i really dislike adding these.  can you explain what is causing it to be needed?
Attached patch workaround-2 (obsolete) — Splinter Review
(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 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)
tracking-fennec: ? → 1.0+
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.
Attachment #400355 - Flags: review?(gavin.sharp) → review-
(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?
Attached patch WIP-1 (obsolete) — Splinter Review
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!
(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.
Attached patch setTimeout workaround patch (obsolete) — Splinter Review
I think we might just want to take this patch for now, and move the WIP patch to a followup bug?
Attachment #399058 - Attachment is obsolete: true
Attachment #400355 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #401013 - Attachment is obsolete: true
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...)
Attachment #401107 - Flags: review?(gavin.sharp)
Attached patch Patch v0.2 (obsolete) — Splinter Review
* replace <xul:caption/> by <hbox><image/><label/></hbox>
Attachment #401107 - Attachment is obsolete: true
Attachment #401844 - Flags: review?(gavin.sharp)
Attachment #401107 - Flags: review?(gavin.sharp)
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.
Attachment #401860 - Attachment description: p → Address comments
Attachment #401860 - Flags: review?(gavin.sharp)
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
Attachment #401844 - Attachment is obsolete: true
Attachment #401844 - Flags: review?(gavin.sharp)
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.
Attachment #401860 - Flags: review?(gavin.sharp) → review+
Attachment #401860 - Attachment is obsolete: true
Attachment #401084 - Attachment is obsolete: true
pushed:
https://hg.mozilla.org/mobile-browser/rev/ee72ab39b952
Assignee: nobody → 21
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.