Closed Bug 464450 Opened 16 years ago Closed 7 years ago

remove "padded" class from type="autocomplete" textboxes without icons

Categories

(Toolkit :: Themes, defect)

Product:
Toolkit
Component:
Themes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mak, Assigned: dao)

References

Details

Attachments

(1 file)

Bug 464450 - Get rid of the padded class and clean up related styling.
59 bytes, text/x-review-board-request
mak
: review+
Details 
actually autocomplete textboxes without icons have to use .padded class defined in autocomplete.css

As stated also in the stylesheet this is more an hack than a fix, so padded should be removed and autocomplete style should be fixed using negative margins for icons and so on.

Quote from Gavin:
">+/* .padded is used by autocomplete widgets that don't have an icon. Gross. -dwh */

Gross indeed! We should get a bug filed on just doing the right thing and
making .padded unnecessary..."
Blocks: 421489
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Depends on: 1407613
Comment on attachment 8918206 [details]
Bug 464450 - Get rid of the padded class and clean up related styling.

https://reviewboard.mozilla.org/r/189088/#review195334

I'm sorry, I have a couple questions to better understand the reach of these changes.

::: toolkit/themes/windows/global/autocomplete.css
(Diff revision 1)
>  
> -/* .padded is used by autocomplete widgets that don't have an icon. Gross. -dwh */
> -textbox:not(.padded) {
> -  cursor: default;
> -  padding: 0;
> -}

could you explain why is it ok to just remove this rule that, off-hand, looks like changing the padding for any textbox that is NOT .padded where we include autocomplete.css (pretty much everywhere there's an autocomplete field)?
Is not this at risk of changing autocomplete and textbox styling all around?
Same question for the margin change below.

::: toolkit/themes/windows/global/autocomplete.css
(Diff revision 1)
> -  margin: 0 3px;
> -}
> -
> -.textbox-input-box {
> -  -moz-box-align: center;
> -}

Isn't the default alignment stretch?
This old center alignment looks it will be used by anything using the basic autocomplete widget. Does it just not matter since we're unlikely to have other children apart from the input box?
(In reply to Marco Bonardo [::mak] from comment #2)
> > -/* .padded is used by autocomplete widgets that don't have an icon. Gross. -dwh */
> > -textbox:not(.padded) {
> > -  cursor: default;
> > -  padding: 0;
> > -}
> 
> could you explain why is it ok to just remove this rule that, off-hand,
> looks like changing the padding for any textbox that is NOT .padded where we
> include autocomplete.css (pretty much everywhere there's an autocomplete
> field)?
> Is not this at risk of changing autocomplete and textbox styling all around?
> Same question for the margin change below.

It makes them use the default textbox spacing. This custom styling that I'm removing is inconsistent with that.

> ::: toolkit/themes/windows/global/autocomplete.css
> (Diff revision 1)
> > -  margin: 0 3px;
> > -}
> > -
> > -.textbox-input-box {
> > -  -moz-box-align: center;
> > -}
> 
> Isn't the default alignment stretch?
> This old center alignment looks it will be used by anything using the basic
> autocomplete widget. Does it just not matter since we're unlikely to have
> other children apart from the input box?

Yeah, I think so. The binding is littered with URL bar specifics that are largely unused since that binding has been forked.
Comment on attachment 8918206 [details]
Bug 464450 - Get rid of the padded class and clean up related styling.

https://reviewboard.mozilla.org/r/189088/#review195414

I did a quick run of a Windows build with the patch applied, and I didn't notice anything strange.
I guess it's worth a try, there's still enough time to handle eventual visual glitches.
Attachment #8918206 - Flags: review?(mak77) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/824ed2525081
Get rid of the "padded" class and clean up related styling. r=mak
Blocks: 1387013
Backed out 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=824ed2525081aa1738968d62bde96a5eb76eea9e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log crashtest: https://treeherder.mozilla.org/logviewer.html#?job_id=137575193&repo=autoland

[task 2017-10-17T14:55:42.563Z] 14:55:42     INFO - [Parent 1000, Main Thread] ###!!! ASSERTION: bad aListVisibleBounds: 'r.GetBounds().IsEqualInterior(aListVisibleBounds)', file /builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp, line 2039
[task 2017-10-17T14:55:42.564Z] 14:55:42     INFO - #01: nsDisplayMask::ComputeVisibility [layout/painting/nsDisplayList.cpp:9072]
[task 2017-10-17T14:55:42.565Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.566Z] 14:55:42     INFO - #02: nsDisplayItem::RecomputeVisibility [layout/painting/nsDisplayList.cpp:2740]
[task 2017-10-17T14:55:42.567Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.568Z] 14:55:42     INFO - #03: mozilla::layers::WebRenderCommandBuilder::GenerateFallbackData [mfbt/RefPtr.h:287]
[task 2017-10-17T14:55:42.569Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.570Z] 14:55:42     INFO - #04: mozilla::layers::WebRenderCommandBuilder::BuildWrMaskImage [mfbt/AlreadyAddRefed.h:121]
[task 2017-10-17T14:55:42.570Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.572Z] 14:55:42     INFO - #05: nsDisplayMask::CreateWebRenderCommands [layout/painting/nsDisplayList.cpp:9165]
[task 2017-10-17T14:55:42.573Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.574Z] 14:55:42     INFO - #06: mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList [gfx/layers/wr/WebRenderCommandBuilder.cpp:226]
[task 2017-10-17T14:55:42.576Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.577Z] 14:55:42     INFO - #07: mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList [gfx/layers/wr/WebRenderCommandBuilder.cpp:219]
[task 2017-10-17T14:55:42.577Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.596Z] 14:55:42     INFO - #08: mozilla::layers::WebRenderCommandBuilder::BuildWebRenderCommands [gfx/layers/wr/WebRenderCommandBuilder.cpp:69]
[task 2017-10-17T14:55:42.602Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.603Z] 14:55:42     INFO - #09: mozilla::layers::WebRenderLayerManager::EndTransactionWithoutLayer [gfx/layers/wr/WebRenderLayerManager.cpp:276]
[task 2017-10-17T14:55:42.604Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.605Z] 14:55:42     INFO - #10: nsDisplayList::PaintRoot [layout/painting/nsDisplayList.cpp:2175]
[task 2017-10-17T14:55:42.606Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.607Z] 14:55:42     INFO - #11: nsLayoutUtils::PaintFrame [mfbt/RefPtr.h:130]
[task 2017-10-17T14:55:42.608Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.610Z] 14:55:42     INFO - #12: mozilla::PresShell::Paint [layout/base/PresShell.cpp:6423]
[task 2017-10-17T14:55:42.610Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.612Z] 14:55:42     INFO - #13: nsViewManager::ProcessPendingUpdatesPaint [gfx/src/nsRegion.h:75]
[task 2017-10-17T14:55:42.612Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.613Z] 14:55:42     INFO - #14: nsViewManager::ProcessPendingUpdatesForView [view/nsViewManager.cpp:408]
[task 2017-10-17T14:55:42.614Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.615Z] 14:55:42     INFO - #15: nsViewManager::ProcessPendingUpdates [view/nsViewManager.cpp:1099]
[task 2017-10-17T14:55:42.616Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.617Z] 14:55:42     INFO - #16: nsRefreshDriver::Tick [layout/base/nsRefreshDriver.cpp:2090]
[task 2017-10-17T14:55:42.618Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.619Z] 14:55:42     INFO - #17: nsRefreshDriver::DoTick [layout/base/nsRefreshDriver.cpp:1529]
[task 2017-10-17T14:55:42.620Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.621Z] 14:55:42     INFO - #18: nsRefreshDriver::FinishedWaitingForTransaction [layout/base/nsRefreshDriver.cpp:2198]
[task 2017-10-17T14:55:42.621Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.622Z] 14:55:42     INFO - #19: mozilla::layers::WebRenderLayerManager::DidComposite [gfx/layers/wr/WebRenderLayerManager.cpp:484]
[task 2017-10-17T14:55:42.623Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.624Z] 14:55:42     INFO - #20: mozilla::layers::CompositorBridgeChild::RecvDidComposite [mfbt/RefPtr.h:78]
[task 2017-10-17T14:55:42.625Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.626Z] 14:55:42     INFO - #21: mozilla::layers::PCompositorBridgeChild::OnMessageReceived [s3:gecko-generated-sources:e953b96bc5aaad9f2107af54a7fd15eba18d6b0621c0ee51412af0d447973e5008c3cec99dbdac6e3ace474b7a4783c4e6e3e04be4fc08896f70bb2b5db98592/ipc/ipdl/PCompositorBridgeChild.cpp::1441]
[task 2017-10-17T14:55:42.627Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.651Z] 14:55:42     INFO - #22: mozilla::ipc::MessageChannel::DispatchAsyncMessage [ipc/glue/MessageChannel.h:635]
[task 2017-10-17T14:55:42.653Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.654Z] 14:55:42     INFO - #23: mozilla::ipc::MessageChannel::DispatchMessage [ipc/glue/MessageChannel.cpp:2051]
[task 2017-10-17T14:55:42.654Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.655Z] 14:55:42     INFO - #24: mozilla::ipc::MessageChannel::RunMessage [ipc/glue/MessageChannel.cpp:1896]
[task 2017-10-17T14:55:42.656Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.657Z] 14:55:42     INFO - #25: mozilla::ipc::MessageChannel::MessageTask::Run [ipc/glue/MessageChannel.cpp:1918]
[task 2017-10-17T14:55:42.658Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.659Z] 14:55:42     INFO - #26: nsThread::ProcessNextEvent [mfbt/Maybe.h:445]
[task 2017-10-17T14:55:42.660Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.661Z] 14:55:42     INFO - #27: NS_ProcessNextEvent [xpcom/threads/nsThreadUtils.cpp:512]
[task 2017-10-17T14:55:42.662Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.663Z] 14:55:42     INFO - #28: nsThread::Shutdown [xpcom/threads/nsThreadUtils.h:328]
[task 2017-10-17T14:55:42.664Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.665Z] 14:55:42     INFO - #29: mozilla::detail::RunnableMethodImpl<nsCOMPtr<nsIThread>, nsresult (nsIThread::*)(), true, (mozilla::RunnableKind)0u>::Run [xpcom/threads/nsThreadUtils.h:1196]
[task 2017-10-17T14:55:42.666Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.667Z] 14:55:42     INFO - #30: nsThread::ProcessNextEvent [mfbt/Maybe.h:445]
[task 2017-10-17T14:55:42.667Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.668Z] 14:55:42     INFO - #31: NS_ProcessNextEvent [xpcom/threads/nsThreadUtils.cpp:512]
[task 2017-10-17T14:55:42.669Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.670Z] 14:55:42     INFO - #32: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:98]
[task 2017-10-17T14:55:42.671Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.672Z] 14:55:42     INFO - #33: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:327]
[task 2017-10-17T14:55:42.673Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.713Z] 14:55:42     INFO - #34: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:298]
[task 2017-10-17T14:55:42.714Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.715Z] 14:55:42     INFO - #35: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:160]
[task 2017-10-17T14:55:42.716Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.717Z] 14:55:42     INFO - #36: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:289]
[task 2017-10-17T14:55:42.718Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.719Z] 14:55:42     INFO - #37: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4695]
[task 2017-10-17T14:55:42.720Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.721Z] 14:55:42     INFO - #38: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4856]
[task 2017-10-17T14:55:42.721Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.722Z] 14:55:42     INFO - #39: XRE_main [toolkit/xre/nsAppRunner.cpp:4951]
[task 2017-10-17T14:55:42.723Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.724Z] 14:55:42     INFO - #40: do_main [browser/app/nsBrowserApp.cpp:231]
[task 2017-10-17T14:55:42.725Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.726Z] 14:55:42     INFO - #41: main [browser/app/nsBrowserApp.cpp:306]
[task 2017-10-17T14:55:42.727Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.728Z] 14:55:42     INFO - #42: libc.so.6 + 0x20830
[task 2017-10-17T14:55:42.729Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.729Z] 14:55:42     INFO - #43: _start
[task 2017-10-17T14:55:42.751Z] 14:55:42     INFO - 
[task 2017-10-17T14:55:42.754Z] 14:55:42     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/docshell/base/crashtests/914521.html | (LOAD ONLY)
[task 2017-10-17T14:55:42.755Z] 14:55:42     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/docshell/base/crashtests/914521.html
[task 2017-10-17T14:55:42.757Z] 14:55:42     INFO - ++DOMWINDOW == 18 (0x7f0997e23000) [pid = 1088] [serial = 65] [outer = 0x7f099abc0800]
[task 2017-10-17T14:55:42.758Z] 14:55:42     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/docshell/base/crashtests/914521.html | assertion count 1 is more than expected 0 assertions

Failure log reftest: https://treeherder.mozilla.org/logviewer.html#?job_id=137590004&repo=autoland
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/autocomplete-1.xul == file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/autocomplete-ref.xul | image comparison, max difference: 192, number of differing pixels: 16626
Flags: needinfo?(dao+bmo)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/7f841ee580b9
Backed out changeset 824ed2525081 for failing reftest editor/reftests/xul/autocomplete-1.xul on Linux builds with stylo disabled and frequently asserting in crashtests, e.g. in docshell/base/crashtests/914521.html. r=backout
Depends on: 1365844
Dão, can you push to try again with a more recent mozilla-central base? I think the platform issue is fixed.
Flags: needinfo?(dao+bmo)
Well, the assertion in the crashtest is fixed. I see now that there was another issue with editor/reftests/xul/autocomplete-1.xul. I don't know anything about that.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa14c955261c
Get rid of the padded class and clean up related styling. r=mak
(In reply to Markus Stange [:mstange] from comment #11)
> Well, the assertion in the crashtest is fixed. I see now that there was
> another issue with editor/reftests/xul/autocomplete-1.xul. I don't know
> anything about that.

I already took care of that after the backout.
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/294c88b3bd2c
Get rid of the padded class and clean up related styling. r=mak
https://hg.mozilla.org/mozilla-central/rev/294c88b3bd2c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1415529
Performance improvements noticed, thanks to this:

== Change summary for alert #10441 (as of Mon, 06 Nov 2017 19:12:02 GMT) ==

Improvements:

  4%  tart summary linux64 opt e10s     5.16 -> 4.98
  1%  tart summary linux64 pgo e10s     4.62 -> 4.57

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10441
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: