Note: There are a few cases of duplicates in user autocompletion which are being worked on.

select.size reflection is wrong

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: mounir)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
In particular, |select.size = -2| shouldn't throw per spec.
(Assignee)

Comment 1

5 years ago
Created attachment 630136 [details] [diff] [review]
Patch v1

With this patch, we are following the specifications and Presto + Webkit behavior.
IE 6 to 10 has our current behavior but given that Presto and Webkit doesn't, we can assume it is safe to follow the specs here.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #630136 - Flags: review?(bugs)

Comment 2

5 years ago
Comment on attachment 630136 [details] [diff] [review]
Patch v1

>+++ b/content/html/content/test/test_bug551846.html
>@@ -98,23 +98,27 @@ function checkSetSizeException(element)
>   caught = false;
>   try {
>     element.size = 0;
>   } catch(e) {
>     caught = true;
>   }
>   ok(!caught, "Setting a size to 0 from the IDL shouldn't throw an exception");
> 
>+  element.size = 1;
>+
>   caught = false;
>   try {
>     element.size = -1;
>   } catch(e) {
>     caught = true;
>   }
>-  ok(caught, "Setting an invalid size from the IDL should throw an exception");
>+  ok(!caught, "Setting a negative size from the IDL shouldn't throw an exception");
>+
>+  is(element.size, 0, "The size should now be equal to the minimum non-negative value");
> 
>   caught = false;
>   try {
>     element.setAttribute('size', -10);
>   } catch(e) {
>     caught = true;
>   }
>   ok(!caught, "Setting an invalid size in the content attribute shouldn't throw an exception");
Please add a check here to ensure that element.size is correct after setAttribute





>diff --git a/dom/interfaces/html/nsIDOMHTMLSelectElement.idl b/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
>--- a/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
>+++ b/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
>@@ -14,25 +14,25 @@
>  * http://www.w3.org/TR/DOM-Level-2-HTML/
>  *
>  * with changes from the work-in-progress WHATWG HTML specification:
>  * http://www.whatwg.org/specs/web-apps/current-work/
>  */
> 
> interface nsIDOMValidityState;
> 
>-[scriptable, uuid(2A50D295-8DB8-4223-AE0D-070C6EB6C76E)]
>+[scriptable, uuid(e85194cf-56e6-44a6-92d9-0096c9d2536e)]
> interface nsIDOMHTMLSelectElement : nsIDOMHTMLElement
> {
>            attribute boolean                     autofocus;
>            attribute boolean                     disabled;
>   readonly attribute nsIDOMHTMLFormElement       form;
>            attribute boolean                     multiple;
>            attribute DOMString                   name;
>-           attribute long                        size;
>+           attribute unsigned long               size;
> 
>   readonly attribute DOMString                   type;
> 
>   readonly attribute nsIDOMHTMLOptionsCollection options;
>            attribute unsigned long               length;
>   nsIDOMNode                item(in unsigned long index);
>   nsIDOMNode                namedItem(in DOMString name);
>   // This add method implementation means the following
>diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
>--- a/layout/base/nsCSSFrameConstructor.cpp
>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -2899,17 +2899,17 @@ nsCSSFrameConstructor::ConstructSelectFr
>   nsresult rv = NS_OK;
>   const PRInt32 kNoSizeSpecified = -1;
> 
>   nsIContent* const content = aItem.mContent;
>   nsStyleContext* const styleContext = aItem.mStyleContext;
> 
>   // Construct a frame-based listbox or combobox
>   nsCOMPtr<nsIDOMHTMLSelectElement> sel(do_QueryInterface(content));
>-  PRInt32 size = 1;
>+  PRUint32 size = 1;
>   if (sel) {
>     sel->GetSize(&size); 
>     bool multipleSelect = false;
>     sel->GetMultiple(&multipleSelect);
>      // Construct a combobox if size=1 or no size is specified and its multiple select
>     if (((1 == size || 0 == size) || (kNoSizeSpecified  == size)) && (false == multipleSelect)) {
>         // Construct a frame-based combo box.
>         // The frame-based combo box is built out of three parts. A display area, a button and
>diff --git a/layout/forms/nsListControlFrame.cpp b/layout/forms/nsListControlFrame.cpp
>--- a/layout/forms/nsListControlFrame.cpp
>+++ b/layout/forms/nsListControlFrame.cpp
>@@ -564,17 +564,17 @@ nsListControlFrame::ReflowAsDropdown(nsP
>   // XXXbz We're just changing the height here; do we need to dirty ourselves
>   // or anything like that?  We might need to, per the letter of the reflow
>   // protocol, but things seem to work fine without it...  Is that just an
>   // implementation detail of nsHTMLScrollFrame that we're depending on?
>   nsHTMLScrollFrame::DidReflow(aPresContext, &state, aStatus);
> 
>   // Now compute the height we want to have
>   mNumDisplayRows = kMaxDropDownRows;
>-  if (visibleHeight > mNumDisplayRows * heightOfARow) {
>+  if (visibleHeight > nscoord(mNumDisplayRows * heightOfARow)) {
>     visibleHeight = mNumDisplayRows * heightOfARow;
>     // This is an adaptive algorithm for figuring out how many rows 
>     // should be displayed in the drop down. The standard size is 20 rows, 
>     // but on 640x480 it is typically too big.
>     // This takes the height of the screen divides it by two and then subtracts off 
>     // an estimated height of the combobox. I estimate it by taking the max element size
>     // of the drop down and multiplying it by 2 (this is arbitrary) then subtract off
>     // the border and padding of the drop down (again rather arbitrary)
>@@ -958,17 +958,17 @@ nsListControlFrame::SetInitialChildList(
>     }
>   }*/
> 
>   return rv;
> }
> 
> //---------------------------------------------------------
> nsresult
>-nsListControlFrame::GetSizeAttribute(PRInt32 *aSize) {
>+nsListControlFrame::GetSizeAttribute(PRUint32 *aSize) {
>   nsresult rv = NS_OK;
>   nsIDOMHTMLSelectElement* selectElement;
>   rv = mContent->QueryInterface(NS_GET_IID(nsIDOMHTMLSelectElement),(void**) &selectElement);
>   if (mContent && NS_SUCCEEDED(rv)) {
>     rv = selectElement->GetSize(aSize);
>     NS_RELEASE(selectElement);
>   }
>   return rv;
>@@ -2398,23 +2398,23 @@ nsListControlFrame::KeyPress(nsIDOMEvent
>         aKeyEvent->PreventDefault(); // since we won't reach the one below
>         return NS_OK;
>       }
>     } break;
> 
>     case nsIDOMKeyEvent::DOM_VK_PAGE_UP: {
>       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
>                                 (PRInt32)numOptions,
>-                                -NS_MAX(1, mNumDisplayRows-1), -1);
>+                                -NS_MAX(1, PRInt32(mNumDisplayRows-1)), -1);
>       } break;
> 
>     case nsIDOMKeyEvent::DOM_VK_PAGE_DOWN: {
>       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
>                                 (PRInt32)numOptions,
>-                                NS_MAX(1, mNumDisplayRows-1), 1);
>+                                NS_MAX(1, PRInt32(mNumDisplayRows-1)), 1);
>       } break;
> 
>     case nsIDOMKeyEvent::DOM_VK_HOME: {
>       AdjustIndexForDisabledOpt(0, newIndex,
>                                 (PRInt32)numOptions,
>                                 0, 1);
>       } break;
> 
>diff --git a/layout/forms/nsListControlFrame.h b/layout/forms/nsListControlFrame.h
>--- a/layout/forms/nsListControlFrame.h
>+++ b/layout/forms/nsListControlFrame.h
>@@ -301,17 +301,17 @@ protected:
>    * those values as determined by the original HTML
>    */
>   virtual void ResetList(bool aAllowScrolling);
> 
>   nsListControlFrame(nsIPresShell* aShell, nsIDocument* aDocument, nsStyleContext* aContext);
>   virtual ~nsListControlFrame();
> 
>   // Utility methods
>-  nsresult GetSizeAttribute(PRInt32 *aSize);
>+  nsresult GetSizeAttribute(PRUint32 *aSize);
>   nsIContent* GetOptionFromContent(nsIContent *aContent);
> 
>   /**
>    * Sets the mSelectedIndex and mOldSelectedIndex from figuring out what 
>    * item was selected using content
>    * @param aPoint the event point, in listcontrolframe coordinates
>    * @return NS_OK if it successfully found the selection
>    */
>@@ -383,17 +383,17 @@ protected:
>     return GetOptionsContainer()->HeightOfARow();
>   }
>   
>   // Data Members
>   PRInt32      mStartSelectionIndex;
>   PRInt32      mEndSelectionIndex;
> 
>   nsIComboboxControlFrame *mComboboxFrame;
>-  PRInt32      mNumDisplayRows;
>+  PRUint32     mNumDisplayRows;
>   bool mChangesSinceDragStart:1;
>   bool mButtonDown:1;
>   // Has the user selected a visible item since we showed the
>   // dropdown?
>   bool mItemSelectionStarted:1;
> 
>   bool mIsAllContentHere:1;
>   bool mIsAllFramesHere:1;
Attachment #630136 - Flags: review?(bugs) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 630136 [details] [diff] [review]
> Patch v1
> 
> >+++ b/content/html/content/test/test_bug551846.html
> >@@ -98,23 +98,27 @@ function checkSetSizeException(element)
> >   caught = false;
> >   try {
> >     element.size = 0;
> >   } catch(e) {
> >     caught = true;
> >   }
> >   ok(!caught, "Setting a size to 0 from the IDL shouldn't throw an exception");
> > 
> >+  element.size = 1;
> >+
> >   caught = false;
> >   try {
> >     element.size = -1;
> >   } catch(e) {
> >     caught = true;
> >   }
> >-  ok(caught, "Setting an invalid size from the IDL should throw an exception");
> >+  ok(!caught, "Setting a negative size from the IDL shouldn't throw an exception");
> >+
> >+  is(element.size, 0, "The size should now be equal to the minimum non-negative value");
> > 
> >   caught = false;
> >   try {
> >     element.setAttribute('size', -10);
> >   } catch(e) {
> >     caught = true;
> >   }
> >   ok(!caught, "Setting an invalid size in the content attribute shouldn't throw an exception");
> Please add a check here to ensure that element.size is correct after
> setAttribute

This is already done in the checkSizeReflection method.
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Attachment #630136 - Flags: checkin+

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/66e43a0eb505
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 769437
You need to log in before you can comment on or make changes to this bug.