Allow passing nils to mw.html

Rather than calling error() when nils get passed to mw.html methods,
either remove whatever it was that the nil would go to (if that makes
sense), or just do nothing. The seemingly inconsistent use of "not x" and
"x ~= nil" is to allow any falsey value where it wouldn't be ambiguous
(such as class names), but not where it could be (such as attribute values).

Bug: 62982
Change-Id: I76773abbb4394aa9bb8c8a08445e019cade3b2bf

engines/LuaCommon/lualib/mw.html.lua

@@ -52,13 +52,13 @@ metatable.__tostring = function( t )
 	return table.concat( ret )
 end
 
--- Get an attribute table (name, value)
+-- Get an attribute table (name, value) and its index
 --
 -- @param name
 local function getAttr( t, name )
 	for i, attr in ipairs( t.attributes ) do
 		if attr.name == name then
-			return attr
+			return attr, i
 		end
 	end
 end
@@ -196,7 +196,7 @@ end
 -- Set an HTML attribute on the node.
 --
 -- @param name Attribute to set, alternative table of name-value pairs
--- @param val Value of the attribute
+-- @param val Value of the attribute. Nil causes the attribute to be unset
 methodtable.attr = function( t, name, val )
 	if type( name ) == 'table' then
 		if val ~= nil then
@@ -219,7 +219,7 @@ methodtable.attr = function( t, name, val )
 	if type( name ) ~= 'string' and type( name ) ~= 'number' then
 		error( 'Invalid name given: The name must be either a string or a number' )
 	end
-	if type( val ) ~= 'string' and type( val ) ~= 'number' then
+	if val ~= nil and type( val ) ~= 'string' and type( val ) ~= 'number' then
 		error( 'Invalid value given: The value must be either a string or a number' )
 	end
 
@@ -234,10 +234,14 @@ methodtable.attr = function( t, name, val )
 		error( "Invalid attribute name: " .. name )
 	end
 
-	local attr = getAttr( t, name )
+	local attr, i = getAttr( t, name )
 	if attr then
-		attr.val = val
-	else
+		if val ~= nil then
+			attr.val = val
+		else
+			table.remove( t.attributes, i )
+		end
+	elseif val ~= nil then
 		table.insert( t.attributes, { name = name, val = val } )
 	end
 
@@ -249,6 +253,10 @@ end
 --
 -- @param class
 methodtable.addClass = function( t, class )
+	if class == nil then
+		return t
+	end
+
 	if type( class ) ~= 'string' and type( class ) ~= 'number' then
 		error( 'Invalid class given: The name must be either a string or a number' )
 	end
@@ -266,7 +274,7 @@ end
 -- Set a CSS property to be added to the node's style attribute.
 --
 -- @param name CSS attribute to set, alternative table of name-value pairs
--- @param val
+-- @param val The value to set. Nil causes it to be unset
 methodtable.css = function( t, name, val )
 	if type( name ) == 'table' then
 		if val ~= nil then
@@ -289,18 +297,24 @@ methodtable.css = function( t, name, val )
 	if type( name ) ~= 'string' and type( name ) ~= 'number' then
 		error( 'Invalid CSS given: The name must be either a string or a number' )
 	end
-	if type( val ) ~= 'string' and type( val ) ~= 'number' then
+	if val ~= nil and type( val ) ~= 'string' and type( val ) ~= 'number' then
 		error( 'Invalid CSS given: The value must be either a string or a number' )
 	end
 
 	for i, prop in ipairs( t.styles ) do
 		if prop.name == name then
-			prop.val = val
+			if val ~= nil then
+				prop.val = val
+			else
+				table.remove( t.styles, i )
+			end
 			return t
 		end
 	end
 
-	table.insert( t.styles, { name = name, val = val } )
+	if val ~= nil then
+		table.insert( t.styles, { name = name, val = val } )
+	end
 
 	return t
 end
@@ -310,11 +324,11 @@ end
 --
 -- @param css
 methodtable.cssText = function( t, css )
-	if type( css ) ~= 'string' and type( css ) ~= 'number' then
-		error( 'Invalid CSS given: Must be either a string or a number' )
-	end
+	if css ~= nil then
+		if type( css ) ~= 'string' and type( css ) ~= 'number' then
+			error( 'Invalid CSS given: Must be either a string or a number' )
+		end
 
-	if css then
 		table.insert( t.styles, css )
 	end
 	return t
@@ -341,12 +355,14 @@ end
 -- @param tagName
 -- @param args
 function HtmlBuilder.create( tagName, args )
-	if type( tagName ) ~= 'string' then
-		error( "Tag name must be a string" )
-	end
+	if tagName ~= nil then
+		if type( tagName ) ~= 'string' then
+			error( "Tag name must be a string" )
+		end
 
-	if tagName ~= '' and not isValidTag( tagName ) then
-		error( "Invalid tag name: " .. tagName )
+		if tagName ~= '' and not isValidTag( tagName ) then
+			error( "Invalid tag name: " .. tagName )
+		end
 	end
 
 	args = args or {}

tests/engines/LuaCommon/HtmlLibraryTests.lua

@@ -55,6 +55,10 @@ local function testAttributeOverride()
 	return getEmptyTestDiv():attr( 'good', 'MediaWiki' ):attr( 'good', 'Wikibase' )
 end
 
+local function testAttributeRemoval()
+	return getEmptyTestDiv():attr( 'a', 'b' ):attr( 'a', nil )
+end
+
 local function testGetAttribute()
 	return getEmptyTestDiv():attr( 'town', 'Berlin' ):getAttr( 'town' )
 end
@@ -75,8 +79,12 @@ local function testWikitextAppendToSelfClosing()
 	return mw.html.create( 'hr' ):wikitext( 'foo' )
 end
 
-local function testEmptyCreate()
-	return mw.html.create( '' ):wikitext( 'foo' ):tag( 'div' ):attr( 'a', 'b' ):allDone()
+local function testCreateWithValue( val )
+	return mw.html.create( val ):wikitext( 'foo' ):tag( 'div' ):attr( 'a', 'b' ):allDone()
+end
+
+local function testCssRemoval()
+	return getEmptyTestDiv():css( 'color', 'red' ):css( 'color', nil )
 end
 
 local function testComplex()
@@ -116,9 +124,6 @@ local tests = {
 	  args = { {} },
 	  expect = 'Tag name must be a string'
 	},
-	{ name = 'mw.html.create (invalid tag 3)', func = mw.html.create, type='ToString',
-	  expect = 'Tag name must be a string'
-	},
 	{ name = 'mw.html.wikitext', func = testHelper, type='ToString',
 	  args = { getEmptyTestDiv(), 'wikitext', 'Plain text' },
 	  expect = { '<div>Plain text</div>' }
@@ -140,6 +145,10 @@ local tests = {
 	  args = { getEmptyTestDiv(), 'attr', 'foo', 'bar' },
 	  expect = { '<div foo="bar"></div>' }
 	},
+	{ name = 'mw.html.attr (nil noop)', func = testHelper, type='ToString',
+	  args = { getEmptyTestDiv(), 'attr', 'foo', nil },
+	  expect = { '<div></div>' }
+	},
 	{ name = 'mw.html.attr (table 1)', func = testHelper, type='ToString',
 	  args = { getEmptyTestDiv(), 'attr', { foo = 'bar' } },
 	  expect = { '<div foo="bar"></div>' }
@@ -196,6 +205,10 @@ local tests = {
 	  args = { getEmptyTestDiv(), 'css', 'foo', 'bar' },
 	  expect = { '<div style="foo:bar;"></div>' }
 	},
+	{ name = 'mw.html.css (nil noop)', func = testHelper, type='ToString',
+	  args = { getEmptyTestDiv(), 'css', 'foo', nil },
+	  expect = { '<div></div>' }
+	},
 	{ name = 'mw.html.css (invalid name 1)', func = testHelper, type='ToString',
 	  args = { getEmptyTestDiv(), 'css', function() end, 'bar' },
 	  expect = 'Invalid CSS given: The name must be either a string or a number'
@@ -244,6 +257,14 @@ local tests = {
 	  args = { getEmptyTestDiv(), 'cssText', 'mu"ha:-ha"ha' },
 	  expect = { '<div style="mu&quot;ha:-ha&quot;ha;"></div>' }
 	},
+	{ name = 'mw.html.addClass (nil)', func = testHelper, type='ToString',
+	  args = { getEmptyTestDiv(), 'addClass' },
+	  expect = { '<div></div>' }
+	},
+	{ name = 'mw.html.cssText (nil)', func = testHelper, type='ToString',
+	  args = { getEmptyTestDiv(), 'cssText' },
+	  expect = { '<div></div>' }
+	},
 
 	-- Tests defined above
 
@@ -274,15 +295,26 @@ local tests = {
 	{ name = 'mw.html.attr (overrides)', func = testAttributeOverride, type='ToString',
 	  expect = { '<div good="Wikibase"></div>' }
 	},
+	{ name = 'mw.html.attr (removal)', func = testAttributeRemoval, type='ToString',
+	  expect = { '<div></div>' }
+	},
 	{ name = 'mw.html.getAttr', func = testGetAttribute, type='ToString',
 	  expect = { 'Berlin' }
 	},
 	{ name = 'mw.html.getAttr (escaping)', func = testGetAttributeEscaping, type='ToString',
 	  expect = { '<ble"&rgh>' }
 	},
-	{ name = 'mw.html.create (empty)', func = testEmptyCreate, type='ToString',
+	{ name = 'mw.html.create (empty string)', func = testCreateWithValue, type='ToString',
+	  args = {''},
 	  expect = { 'foo<div a="b"></div>' }
 	},
+	{ name = 'mw.html.create (nil)', func = testCreateWithValue, type='ToString',
+	  args = {nil},
+	  expect = { 'foo<div a="b"></div>' }
+	},
+	{ name = 'mw.html.css (removal)', func = testCssRemoval, type='ToString',
+	  expect = { '<div></div>' }
+	},
 	{ name = 'mw.html complex test', func = testComplex, type='ToString',
 	  expect = {
 		'<div class="firstClass" what="ever"><meh whynot="Русский"><hr a="b" /></meh>' ..