Currently, listbox padding is modeled similarly as the border of the listbox (meaning that content scrolls within it, but the padding does not move when scrolling). This is not consistent with other browsers, and is not consistent with the CSS box model. Instead, padding should move when you scroll the listbox. It should conceptually move with the inner contents of the list box.
One solution for this bug would be to migrate list boxes on top of RenderLayer. That would also fix https://bugs.webkit.org/show_bug.cgi?id=156587
Created attachment 276639 [details] Patch v0 - For EWS (no tests yet) I have given a shot at it over the weekend, and believe to have got a patch that brings WebKit to behave even closer to other Web engines (e.g. Firefox and specially Chrome) do, with regards to listbox'es and padding. Patch v0 (no tests yet), also addresses bug 156591.
Comment on attachment 276639 [details] Patch v0 - For EWS (no tests yet) View in context: https://bugs.webkit.org/attachment.cgi?id=276639&action=review > Source/WebCore/rendering/RenderListBox.cpp:93 > + , m_firstItemVisibleOverPaddingTop(-1) Usually we use a pattern of Optional<int> (or, even better, Optional<unsigned>) instead of using magic "-1" values.
Created attachment 277062 [details] Patch v1
Comment on attachment 277062 [details] Patch v1 Tests should be skipped for iOS, since multiline listboxes is not supported.
Comment on attachment 277062 [details] Patch v1 Attachment 277062 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1202527 New failing tests: fast/forms/listbox-top-padding-do-not-clip-items.html fast/replaced/table-percent-height.html
Created attachment 277065 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 277085 [details] Patch v1.1 - skipped tests in iOS. v1.1 skips the tests for iOS, since multiline listboxes are handled as dropdown.
Created attachment 277086 [details] Patch v1.2 - removed wrongly added PNG file.
(In reply to comment #1) > One solution for this bug would be to migrate list boxes on top of > RenderLayer. That would also fix > https://bugs.webkit.org/show_bug.cgi?id=156587 I believe this specific bug is worth it to get fixed, before we actually implement RenderListBox on top of RenderLayer. This is a short/mid term solution, that makes WebKit match Blink with regarding to respect top/bottom paddings.
Created attachment 277270 [details] Patch 1.3 - better method names
Created attachment 277275 [details] Patch v1.4 - better ChangeLog
Comment on attachment 277275 [details] Patch v1.4 - better ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=277275&action=review > LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt:4 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +FAIL i > 8 && i < max_attempts should be true. Was false. This test fails. Is this expected? Moreover the output of this test does not read well. Specifically, the PASS/FAIL message is printed after the TEST COMPLETE message because we explicitly call window.notifyDone() in the test instead of setting window.jsTestIsAsync = true and calling finishJSTest(). > LayoutTests/fast/forms/listbox-respects-padding-bottom.html:9 > + <style type="text/css"> > + :focus { > + outline: 0; > + } > + </style> Is this necessary? > LayoutTests/fast/forms/listbox-respects-padding-bottom.html:13 > + if (window.testRunner) > + testRunner.waitUntilDone(); > + We are underutilizing js-test-{pre, post}.js in this test. We should set window.jsTestAsync = true here and call finishJSTest() to signal test completion instead of using testRunner.waitUntilDone()/testRunner.notifyDone(). We should also consider calling description() with a description of the purpose of this test. > LayoutTests/fast/forms/listbox-respects-padding-bottom.html:37 > + var i = 0; > + var max_attempts = 10; > + function runTest() > + { > + var scrollAmount = 0; > + var sl = document.getElementById('sl'); > + sl.scrollTop = sl.scrollHeight; > + var maxScrollOffset = sl.scrollTop; > + > + var x = sl.offsetLeft + (sl.offsetLeft + sl.offsetWidth) / 2; > + var y = sl.offsetTop + sl.offsetHeight - 15; > + > + for ( ; i < max_attempts; i++) { > + var el = document.elementFromPoint(x, y); > + if (!(el instanceof HTMLOptionElement)) { > + scrollAmount += 10; > + sl.scrollTop = maxScrollOffset - scrollAmount; > + } else break; > + } > + > + shouldBeTrue("i > 8 && i < max_attempts"); We tend to follow the WebKit Code Style Guidelines for JavaScript code in tests. Please update this to conform to the style guidelines. > LayoutTests/fast/forms/listbox-respects-padding-bottom.html:43 > + <body onload="setTimeout(runTest, 0)"> How did you come to the decision to use a zero timer? Assuming the purpose of this zero timer is to ensure we have completed a layout, can we replace the use of this timer with internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks()? > LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items-expected.txt:5 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +PASS i > 0 && i < max_attempts is true > + This output does not read well. Specifically, the PASS/FAIL message is printed after the TEST COMPLETE message. See my remark for the test LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt for more details. > LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items.html:4 > +<!DOCTYPE html> > +<html> > + <script src="../../resources/js-test-pre.js"></script> > + <head> Can we/would it make sense to share more code with the above test, listbox-respects-padding-bottom.html? > Source/WebCore/ChangeLog:26 > + In short, dpending on the scroll position and the amount of space available on the padding top/bottom Nit: dpending => depending > Source/WebCore/ChangeLog:45 > + (WebCore::RenderListBox::scrollTo): Where are the changes to this function? > Source/WebCore/rendering/RenderListBox.cpp:245 > int RenderListBox::numVisibleItems() const > { > // Only count fully visible rows. But don't return 0 even if only part of a row shows. > - return std::max<int>(1, (contentHeight() + paddingBottom() + rowSpacing) / itemHeight()); > + return std::max<int>(1, (contentHeight() + rowSpacing) / itemHeight()); > } Now that we are counting the number of items inside padding top of the list box, the content area of the list box, and the padding bottom of the list box, the name of this function is disingenuous and the comment on line 243 is misleading. This function now returns the number of fully visible rows with respect to the content inset described by padding top and padding bottom. Maybe a better name for this function would be numberOfVisibleItemsExcludingPaddingTopAndPaddingBottom()? Or numberOfVisibleItemsIgnoringPadding()? Or numberOfVisibleItemsIgnoringPaddingTopAndPaddingBottom()? > Source/WebCore/rendering/RenderListBox.cpp:285 > + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; We should write this using Optional::valueOr(). > Source/WebCore/rendering/RenderListBox.cpp:288 > + while (index < listItemsSize && index < firstVisibleItem + numVisibleItems) { Minor: Maybe it would be better to write this as a for-loop? > Source/WebCore/rendering/RenderListBox.cpp:310 > + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; We should write this using Optional::valueOr(). > Source/WebCore/rendering/RenderListBox.cpp:313 > + while (index < listItemsSize && index < firstVisibleItem + numVisibleItems) { Minor: Maybe it would be better to write this as a for-loop? On another note, I wish there was a way for use to avoid duplicating the same computations in this paint phase as we did in phase PaintPhaseForeground. Can we/would it make sense to share more code? > Source/WebCore/rendering/RenderListBox.cpp:599 > + if (index >= *m_firstItemVisibleOverPaddingTop && index < m_indexOffset) I would suggest that we using Optional::value() instead of making use of the dereference operator to get the value of the Optional as the latter gives the impression that m_firstItemVisibleOverPaddingTop is either a raw pointer or a smart pointer type. Maybe we should ask on webkit-dev and update the Code Style Guidelines on the preferred way to access the value of an Optional? > Source/WebCore/rendering/RenderListBox.cpp:662 > +int RenderListBox::numberOfItemsAllowedToBePaintedOverPaddingTop() const > +{ > + return floor(static_cast<float>(paddingTop()) / static_cast<float>(itemHeight())); > +} > + > +int RenderListBox::numberOfItemsAllowedToBePaintedOverPaddingBottom() const > +{ > + return floor(static_cast<float>(paddingBottom()) / static_cast<float>(itemHeight())); > +} > + The names of these function doe not read well and eschews the adjective "visible" that we use to refer to the items to be painted. Are these functions necessary given that m_firstItemVisibleOverPaddingTop and m_firstItemVisibleOverPaddingBottom are Optionals? I mean, can we make use of the Optional-ness of these data types such that we can inline the computations performed by these function into calculateIndexesPaintedOverPadding()? > Source/WebCore/rendering/RenderListBox.cpp:663 > +int RenderListBox::numberOfItemsPaintedOverPaddingTop() const The name of this function is inconsistent with the terminology used throughout this class to refer to painted items ("visible items"). Maybe a better name for this function would be numberOfVisibleItemsThatFitInsidePaddingTop()? Or numberOfVisibleItemsInPaddingTop()? I am not happy with either of these. > Source/WebCore/rendering/RenderListBox.cpp:665 > + if (!m_firstItemVisibleOverPaddingTop || !numberOfItemsAllowedToBePaintedOverPaddingTop()) Can you elaborate a case where m_firstItemVisibleOverPaddingTop is non-Nullopt and numberOfItemsCanIntrudePaddingTop() returns 0? > Source/WebCore/rendering/RenderListBox.cpp:671 > +int RenderListBox::numberOfItemsPaintedOverPaddingBottom() const The name of this function is inconsistent with the terminology used throughout this class to refer to painted items ("visible items"). Maybe a better name for this function would be numberOfVisibleItemsThatFitInsidePaddingBottom()? Or numberOfVisibleItemsInPaddingBottom()? I am not very happy with either of these. > Source/WebCore/rendering/RenderListBox.cpp:676 > + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; We should write this using Optional::valueOr(). > Source/WebCore/rendering/RenderListBox.h:175 > + Optional<int> m_firstItemVisibleOverPaddingTop; The name of this variable is disingenuous. This is not an item. This is an index that corresponds to an item. Maybe m_indexOfFirstVisibleItemInsidePaddingTop? Or m_indexOfFirstVisibleItemInsidePaddingTopArea? > Source/WebCore/rendering/RenderListBox.h:176 > + Optional<int> m_firstItemVisibleOverPaddingBottom; Similarly, the name of this variable is disingenuous. Maybe a better name would be m_indexOfFirstVisibleItemInsidePaddingBottom? Or m_indexOfFirstVisibleItemInsidePaddingBottomArea?
Comment on attachment 277275 [details] Patch v1.4 - better ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=277275&action=review Thanks for the great review, Dan. I have addressed your comments in "Patch v2". >> LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt:4 >> +FAIL i > 8 && i < max_attempts should be true. Was false. > > This test fails. Is this expected? > > Moreover the output of this test does not read well. Specifically, the PASS/FAIL message is printed after the TEST COMPLETE message because we explicitly call window.notifyDone() in the test instead of setting window.jsTestIsAsync = true and calling finishJSTest(). It is not expected to fail. I have fixed this test. Also changed it to use "jsTestIsAsync" + "finishJSTest" idiom (fixing its outcome), and added a description() call. >> LayoutTests/fast/forms/listbox-respects-padding-bottom.html:9 >> + </style> > > Is this necessary? No, removed. >> LayoutTests/fast/forms/listbox-respects-padding-bottom.html:13 >> + > > We are underutilizing js-test-{pre, post}.js in this test. We should set window.jsTestAsync = true here and call finishJSTest() to signal test completion instead of using testRunner.waitUntilDone()/testRunner.notifyDone(). We should also consider calling description() with a description of the purpose of this test. Test changed to use "jsTestIsAsync" + "finishJSTest" idiom (fixing its outcome), and I also added a description() call. >> LayoutTests/fast/forms/listbox-respects-padding-bottom.html:37 >> + shouldBeTrue("i > 8 && i < max_attempts"); > > We tend to follow the WebKit Code Style Guidelines for JavaScript code in tests. Please update this to conform to the style guidelines. Done. >> LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items-expected.txt:5 >> + > > This output does not read well. Specifically, the PASS/FAIL message is printed after the TEST COMPLETE message. See my remark for the test LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt for more details. Fixed. >> LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items.html:4 >> + <head> > > Can we/would it make sense to share more code with the above test, listbox-respects-padding-bottom.html? I believe it would make sense, yes. I left the two tests separate in order to isolate the scenario that which one tests. Hope it is ok with you. >> Source/WebCore/ChangeLog:26 >> + In short, dpending on the scroll position and the amount of space available on the padding top/bottom > > Nit: dpending => depending Fixed. >> Source/WebCore/ChangeLog:45 >> + (WebCore::RenderListBox::scrollTo): > > Where are the changes to this function? There is a call too calculateIndexesPainterOverPadding added. >> Source/WebCore/rendering/RenderListBox.cpp:245 >> } > > Now that we are counting the number of items inside padding top of the list box, the content area of the list box, and the padding bottom of the list box, the name of this function is disingenuous and the comment on line 243 is misleading. This function now returns the number of fully visible rows with respect to the content inset described by padding top and padding bottom. Maybe a better name for this function would be numberOfVisibleItemsExcludingPaddingTopAndPaddingBottom()? Or numberOfVisibleItemsIgnoringPadding()? Or numberOfVisibleItemsIgnoringPaddingTopAndPaddingBottom()? Very true. I have renamed it to numberOfVisibleItemsIgnoringPadding(). >> Source/WebCore/rendering/RenderListBox.cpp:285 >> + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; > > We should write this using Optional::valueOr(). Done. >> Source/WebCore/rendering/RenderListBox.cpp:310 >> + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; > > We should write this using Optional::valueOr(). Done. >> Source/WebCore/rendering/RenderListBox.cpp:313 >> + while (index < listItemsSize && index < firstVisibleItem + numVisibleItems) { > > Minor: Maybe it would be better to write this as a for-loop? > > On another note, I wish there was a way for use to avoid duplicating the same computations in this paint phase as we did in phase PaintPhaseForeground. Can we/would it make sense to share more code? We can definitively share this code. Mind if I do that in a follow up? >> Source/WebCore/rendering/RenderListBox.cpp:599 >> + if (index >= *m_firstItemVisibleOverPaddingTop && index < m_indexOffset) > > I would suggest that we using Optional::value() instead of making use of the dereference operator to get the value of the Optional as the latter gives the impression that m_firstItemVisibleOverPaddingTop is either a raw pointer or a smart pointer type. Maybe we should ask on webkit-dev and update the Code Style Guidelines on the preferred way to access the value of an Optional? Done. Will send a query to webkit-dev today too. >> Source/WebCore/rendering/RenderListBox.cpp:662 >> + > > The names of these function doe not read well and eschews the adjective "visible" that we use to refer to the items to be painted. Are these functions necessary given that m_firstItemVisibleOverPaddingTop and m_firstItemVisibleOverPaddingBottom are Optionals? I mean, can we make use of the Optional-ness of these data types such that we can inline the computations performed by these function into calculateIndexesPaintedOverPadding()? This is a great idea. I were are able completely inline numberOfItemsAllowedToBePaintedOverPaddingTop() into calculateIndexesPaintedOverPadding() and remove the declaration. In case of numberOfItemsAllowedToBePaintedOverPaddingBottom() , it is called from two different methods, so I kept it with a different name: maximumNumberOfItemsThatFitInPaddingBottomArea(). >> Source/WebCore/rendering/RenderListBox.cpp:665 >> + if (!m_firstItemVisibleOverPaddingTop || !numberOfItemsAllowedToBePaintedOverPaddingTop()) > > Can you elaborate a case where m_firstItemVisibleOverPaddingTop is non-Nullopt and numberOfItemsCanIntrudePaddingTop() returns 0? That should not happen, indeed. Removed the check. >> Source/WebCore/rendering/RenderListBox.cpp:671 >> +int RenderListBox::numberOfItemsPaintedOverPaddingBottom() const > > The name of this function is inconsistent with the terminology used throughout this class to refer to painted items ("visible items"). Maybe a better name for this function would be numberOfVisibleItemsThatFitInsidePaddingBottom()? Or numberOfVisibleItemsInPaddingBottom()? I am not very happy with either of these. Good point. I renamed it to numberOfVisibleItemsInPaddingBottom(). >> Source/WebCore/rendering/RenderListBox.cpp:676 >> + int firstVisibleItem = m_firstItemVisibleOverPaddingTop ? *m_firstItemVisibleOverPaddingTop : m_indexOffset; > > We should write this using Optional::valueOr(). Done. >> Source/WebCore/rendering/RenderListBox.h:175 >> + Optional<int> m_firstItemVisibleOverPaddingTop; > > The name of this variable is disingenuous. This is not an item. This is an index that corresponds to an item. Maybe m_indexOfFirstVisibleItemInsidePaddingTop? Or m_indexOfFirstVisibleItemInsidePaddingTopArea? Good point. Renamed it to m_indexOfFirstVisibleItemInsidePaddingTopArea. >> Source/WebCore/rendering/RenderListBox.h:176 >> + Optional<int> m_firstItemVisibleOverPaddingBottom; > > Similarly, the name of this variable is disingenuous. Maybe a better name would be m_indexOfFirstVisibleItemInsidePaddingBottom? Or m_indexOfFirstVisibleItemInsidePaddingBottomArea? Renamed it to m_indexOfFirstVisibleItemInsidePaddingBottomArea.
Created attachment 277477 [details] Patch v2 - addressed Dan's review.
Comment on attachment 277477 [details] Patch v2 - addressed Dan's review. View in context: https://bugs.webkit.org/attachment.cgi?id=277477&action=review > Source/WebCore/rendering/RenderListBox.cpp:291 > - int index = m_indexOffset; > - while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { > + int firstVisibleItem = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset); > + int index = firstVisibleItem; > + int numVisibleItems = numberOfVisibleItemsInPaddingTop() + numberOfVisibleItemsIgnoringPadding() + numberOfVisibleItemsInPaddingBottom(); > + while (index < listItemsSize && index < firstVisibleItem + numVisibleItems) { > paintItemForeground(paintInfo, paintOffset, index); > index++; > } Seems really unfortunate that we repeat this relatively complex code twice. Can we factor this out somehow without losing efficiency? > Source/WebCore/rendering/RenderListBox.cpp:653 > + return floor(static_cast<float>(paddingBottom()) / static_cast<float>(itemHeight())); This is inefficient. First it converts the two integers (or is it layout units) to floats, then does a float division, then converts the result of that division from a float to a double to pass to the floor function, then does a floor operation on the double, then converts the double to an integer, which includes the floor operation as all double to integer conversions do. I believe we would get the same result, more efficiently, from doing the arithmetic with integers or layout units. Something more like this: return paddingBottom() / itemHeight(); Can you find any example of values where this does not work? What guarantees do we have about the values? Can the padding or item height be a huge number that doesn’t fit in an integer? Can the item height be 0? What does the function do in those cases? If floating point does need to be involved, we can probably still do better by not using floor, which introduces an unnecessary conversion to double and an extra unneeded floor. If these are layout units rather than integers, then I think we should use the function to convert to float, not a static_cast. > Source/WebCore/rendering/RenderListBox.cpp:670 > + return std::fmin(maximumNumberOfItemsThatFitInPaddingBottomArea(), numItems() - firstVisibleItem - numberOfVisibleItemsInPaddingTop() - numberOfVisibleItemsIgnoringPadding()); Why are we using std::fmin instead of std::min here?
Comment on attachment 277477 [details] Patch v2 - addressed Dan's review. (In reply to comment #16) > > (..) > > > Source/WebCore/rendering/RenderListBox.cpp:291 > > - int index = m_indexOffset; > > - while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { > > + int firstVisibleItem = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset); > > + int index = firstVisibleItem; > > + int numVisibleItems = numberOfVisibleItemsInPaddingTop() + numberOfVisibleItemsIgnoringPadding() + numberOfVisibleItemsInPaddingBottom(); > > + while (index < listItemsSize && index < firstVisibleItem + numVisibleItems) { > > paintItemForeground(paintInfo, paintOffset, index); > > index++; > > } > > Seems really unfortunate that we repeat this relatively complex code twice. > Can we factor this out somehow without losing efficiency? Yes, I have uploaded a patch to bug 157117. > > Source/WebCore/rendering/RenderListBox.cpp:653 > > + return floor(static_cast<float>(paddingBottom()) / static_cast<float>(itemHeight())); > > This is inefficient. First it converts the two integers (or is it layout > units) to floats, then does a float division, then converts the result of > that division from a float to a double to pass to the floor function, then > does a floor operation on the double, then converts the double to an > integer, which includes the floor operation as all double to integer > conversions do. > > I believe we would get the same result, more efficiently, from doing the > arithmetic with integers or layout units. Something more like this: > > return paddingBottom() / itemHeight(); > > Can you find any example of values where this does not work? > > > What guarantees do we have about the values? Can the padding or item height > be a huge number that doesn’t fit in an integer? Can the item height be 0? > What does the function do in those cases? > > If floating point does need to be involved, we can probably still do better > by not using floor, which introduces an unnecessary conversion to double and > an extra unneeded floor. > > If these are layout units rather than integers, then I think we should use > the function to convert to float, not a static_cast. You are right about the many unneeded casts. I used static_cast to float, and then an explicit call to floor() because of the original approach I took for this patch. As things evolved, this turns out to not be needed, but I did not update this method. Will change in a newer version of this patch. > > Source/WebCore/rendering/RenderListBox.cpp:670 > > + return std::fmin(maximumNumberOfItemsThatFitInPaddingBottomArea(), numItems() - firstVisibleItem - numberOfVisibleItemsInPaddingTop() - numberOfVisibleItemsIgnoringPadding()); > > Why are we using std::fmin instead of std::min here? Again, I was working with float in an old version of this patch, so fmin. min is enough now. Will change in a newer version of this patch.
Created attachment 277628 [details] Patch v3 - Addressed Darin's + Dan's offline review. Patch v3: - avoid duplication mentioned by Darin in comment #16, thanks for bug 157117. - avoid the implicit type conversion churn mentioned by Darin in comment #16. - Fixed style issues in the LayoutTests (JS code mostly). - simplified the math used throughout the logic. - Avoided unneeded changes by keeping numVisibleItems() name.
Created attachment 277632 [details] Patch v3.1 - Addressed Darin's + Dan's offline review + simpler math.
Comment on attachment 277632 [details] Patch v3.1 - Addressed Darin's + Dan's offline review + simpler math. View in context: https://bugs.webkit.org/attachment.cgi?id=277632&action=review > Source/WebCore/rendering/RenderListBox.cpp:244 > + int visibleItemsExcludingPadding = std::max<int>(1, (contentHeight() + rowSpacing) / itemHeight()); In WebKit coding style we would not include two space between "=" and "std". > Source/WebCore/rendering/RenderListBox.h:164 > + enum class ConsiderPadding { Yes , No }; In WebKit coding style, there’s no space before the comma.
Created attachment 277732 [details] Patch v3.2 - For landing.
Comment on attachment 277732 [details] Patch v3.2 - For landing. Clearing flags on attachment: 277732 Committed r200265: <http://trac.webkit.org/changeset/200265>
All reviewed patches have been landed. Closing bug.